ext/phar: Do some refactoring to make the code easier to understand #15426
Conversation
Girgias
force-pushed
the
phar-refactoring
branch
4 times, most recently
from
0619888 to
aea37bf
Compare
ext/phar/phar_object.c
Outdated
| } | ||
|
|
||
| if (ZSTR_LEN(new_alias) == 0) { | ||
| zend_argument_value_error(1, "cannot be empty"); |
There was a problem hiding this comment.
Use that new fancy helper function with consistent wording please.
ext/phar/phar_object.c
Outdated
| int old_temp, readd = 0; | ||
|
|
||
| if (zend_parse_parameters(ZEND_NUM_ARGS(), "s", &alias, &alias_len) == FAILURE) { | ||
| // TODO Should this be using a "P" modifier? |
There was a problem hiding this comment.
Possibly, but let's not concern ourselves with that in this PR.
ext/phar/phar_object.c
Outdated
| RETURN_THROWS(); | ||
| } | ||
|
|
||
| if (ZSTR_LEN(new_alias) == 0) { |
There was a problem hiding this comment.
Adding this check will make it impossible to unset an alias, i.e. you can now do setAlias("") to throw away the alias. I think that behaviour should be kept because there's no other way to do it otherwise.
There was a problem hiding this comment.
Right, I am confused how this is handled tho? As it feels like if you try to call setAlias() on two different `PharObject's you'll get an exception? But I didn't try it nor added tests, so I'm shunting this for later.
There was a problem hiding this comment.
Right, I am confused how this is handled tho? As it feels like if you try to call setAlias() on two different `PharObject's you'll get an exception?
I don't understand the question
There was a problem hiding this comment.
From a quick glance it seems like trying to set an empty string as an alias for two different PharObjects would cause a PharException, but I might be missing something.
| zend_throw_exception_ex(spl_ce_BadMethodCallException, 0, "Cannot create any files in magic \".phar\" directory"); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| /* TODO How to handle Windows path normalisation with zend_string ? */ |
There was a problem hiding this comment.
This isn't that bad compared to some other stuff I've seen
ext/phar/phar_object.c
Outdated
| php_stream *resource; | ||
| zval zresource; | ||
|
|
||
| if (zend_parse_parameters(ZEND_NUM_ARGS(), "p|s!", &fname, &fname_len, &localname, &localname_len) == FAILURE) { | ||
| // TODO Should local_name use "P" modifier? |
And remove useless casts
They were only used there, therefore mark them static
Girgias
force-pushed
the
phar-refactoring
branch
from
aea37bf to
15babe5
Compare
Girgias
force-pushed
the
phar-refactoring
branch
from
15babe5 to
697664d
Compare
…nges Follow-up to phpGH-15426 (d99dc30, 6836cae)
…nges Follow-up to phpGH-15426 (6836cae)
…nges Follow-up to phpGH-15426 (6836cae) Co-Authored-By: Gina Peter Banyard <girgias@php.net>
No description provided.