Use zend_string for phar alias #17511
Conversation
nielsdos
force-pushed
the
phar-less-copies-3
branch
from
e088c3b to
0379bbd
Compare
| // TODO: what was the point of the fname != alias check? Adding a check for the byte contents make some cache_list tests fail... | ||
| if (phar_obj->archive->alias/* && !zend_string_equals_cstr(phar_obj->archive->alias, phar_obj->archive->fname, phar_obj->archive->fname_len)*/) { |
There was a problem hiding this comment.
Lovely, this feels like it supposed the alias shared the pointer with an fname, which seems weird. But surely if the alias is equal to the filename then it is not really an alias...
There was a problem hiding this comment.
Yeah it's almost as if the check is actually checking whether the alias was ever changed manually, but I'm not sure and this code has been here since the beginning. In classic PHP fashion the alias functionality is also underspecified and the impact of the branch is not tested.
There was a problem hiding this comment.
You wouldn't know the behaviour of this by any chance @theofidry ?
There was a problem hiding this comment.
Apologies only saw that now.
This seems really obscure and confusing to me. So I do not have any clue unfortunately.
it's almost as if the check is actually checking whether the alias was ever changed manually
I fail to understand (I don't really understand this piece of C code) why it is of any relevance, but in case this is not known (doubt it, but better safe than sorry), the alias can indeed be changed at any time, including after the Phar object was built.
There was a problem hiding this comment.
I fail to understand (I don't really understand this piece of C code) why it is of any relevance, but in case this is not known (doubt it, but better safe than sorry), the alias can indeed be changed at any time, including after the Phar object was built.
Thanks for the input
Yes indeed. This is why the check is so strange.
I'd opt to just drop the check, it doesn't make sense and no test is influenced. If it breaks anyway, CI nightly or early adopters might catch it (and we still have like half a year to catch this).
No description provided.