-
Notifications
You must be signed in to change notification settings - Fork 7.9k
feat: allow empty extension name, fixes #19812 #19816
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Huh, that's a really cool idea.
Looks reasonable. Do you mind shooting a quick e-mail to the internals mailing list to ask for feedback? Technically this may still be considered for 8.5 RC1.
This is already the case. The empty string is ignored, that's why you see the diagnostic message.
What does it diagnose and give you the warning for the benefit: the empty string. It's just not necessary -- don't send an empty directive, it's void. The warning only tells you this already (and not the three+ other permuations that are possible for a non-empty stirng)
The only alternative I can think about is existing with code 254 to fail fast because setting a directive to empty should be diagnosesd as E_DEPRACATED like a since PHP 8.4 function parameter that is just = null without telling per it's type that NULL is included for it's value. Directives are string per their type per the INI directives (at least never NULL) and henceforece PHP 8.4 should have given an E_DEPRECATED error, therefore master should already have rejected with a non-zero status code (likely 254 as it's early) and shutdown.
My 2 cents.
And to add this report is wrong for the extensions named, the directive is zend_extension
not extension
(as given).
This is already the case. The empty string is ignored, that's why you see the diagnostic message.
What do you mean? This is the same message shown for any non-existent extension.
don't send an empty directive, it's void.
Did you read the original issue? The aim is to have something like:
{if hasEnv("FOO")}
extension=foo.so
{/if}
without having to invent new syntax. Tim suggested a simple approach of ignoring empty extension
directives. As mentioned on the list, if the fear is that we might accidentally pass empty environment variables without warning, we could use a different sentinel value. Inventing a new if
conditional would be significantly more involved, though also solve more problems.
Yes, right, and rightly so. If you give PHP the directive to try a null string for the directive it resolves it as a relative pathname. PHP does not stop to load here. Don't provide it if you don't mean it.
Yes I did, this is the equivalent of .PHONY : conditional conditional : # If people set these on the command-line, use them PHP ?= php PHPFLAGS ?= ifdef FOO PHPFLAGS += --define extension=foo.so endif conditional : $(PHP) $(PHPFLAGS) -r 'printf("Hello, world!\n");' Biased towards an in-work-tree invocation, and it shows more clearly what the extension directive stands for: load the extension and with it, load the extensions' directives. This makes the two directives, extension and zend_extension, special. The equivalent at the entry-point of a system image packaged FROM the Docker library is much shorter and works since PHP 5.4: if [ "$FOO" ] then PHP_INI_SCAN_DIR="$PHP_INI_DIR/cfg-foo:$PHP_INI_SCAN_DIR" fi Note the colon ":" in the listing; the empty entry stands for the compile time value (ex. "$PHP_INI_DIR/conf.d" in a PHP container from the library). You may want to flip the order (append instead of prepend) or entirely set/replace. Table: Environment Parameter at the Entry-Point affecting PHP Resource Configuration and Scanning for INI-Files
To complete the example: $ cat "$PHP_INI_DIR/cfg-foo/foo.ini" extension = foo.so The OPs' configuration directive, now conditionally parsed based on the FOO environment parameter. Problem solved. Otherwise, there is /tmp in a read-only container. Nevertheless, to be clear: Our problem is not a read-only file-system here, from a systems engineering perspective we were longtime coming from the standpoint that files in sysconfigdir are not changed by the user nor the program. This is also the reason why the OP's-known solution actually is technically wrong when packaging the system image, and we should avoid any potential x/y problems here.
Yes, right, and if we still want to patch the configuration loading I'd recommend to add two new directives:
they only delegate to their sibling if the value on the right-hand side represents a non-empty string; that is:
This helps to clearly differentiate between their ground form and the intended use, that is to achieve the command-line directive semantics as outlined in the Makefile listing for PHPFLAGS above. There is also no need to find a sentinel value; I think empty is the best value for what we want to express here, especially as we have to map environment parameter effective and efficiently. Other directives should not be affected: we use extension and zend_extension to shortcut the effectiveness of the extensions configuration directives (if it could be loaded) so we move the condition to the place where it belongs in the logical order of the abstractions. flowchart LR
A((start))
A --> C
A --> d1
A --> d2
d1(extension_ifdef) ---> B
B{defined?} -- no --> C
C((stop))
B -- yes --> d2
d2(extension)
d2 --> C
Furthermore, and additionally I propose we should add another long option and an ini-file counterpart. The [PHP] example_directive:env = ENVVAR The right hand side evaluates to the name of an environment variable. It is an error if it does not exist in the environment. Consequences of an error in these instances: The runtime stops loading the configuration and exits with a non-zero exit status not larger than 254, including. To share the benefit of the new error handling within the configuration, I'd further-furthermore and additionally suggest to have ':?'/'?' semantics with the environment parameter expansion in php-ini-files: example_directive = ${ENVVAR?} Given ENVVAR does not exist in the environment, it is an error with the same consequences as already outlined. example_directive = ${ENVVAR:?} Given ENVVAR does not exist in the environment or is null, it is an error with the same consequences as already outlined. |
Allows to load a PHP extension by a php.ini like:
extension=${PROFILER}
If PROFILER env is not there, it loads nothing. if there. it loads that extension