Skip to content

Conversation

Misterio77
Copy link
Contributor

@Misterio77 Misterio77 commented May 30, 2023

Hi, all!

This PR implements fallback/default values when substituting variables on ini files, like: ${VAR:-fallback}. I was actually surprised this wasn't implemented yet, as it's a very useful feature.

I know a thing or two about lexers/parsers, but it's not something I'm super familiar with, so let me know if there's any way this can be improved!

At the moment, the fallback value is treated similarly to a raw string, so it does not support arbitrary expressions (such as nested variables), I couldn't get it to without adding state pushes and pops everywhere on the lexer. It works similarly to offsets/sections now, and supports nesting!

As I understand, this should go through the RFC process? If so, I'm totally okay with writing one :). Let me know if there's any etiquette I might have missed, too.


As an example, my usecase is using environment variables to configure php-fpm, while also having some sane defaults. Like so:

error_log = syslog
daemonize = false

[www]
listen = localhost:${DRUPAL_FPM_PORT:-9000}

@Misterio77 Misterio77 force-pushed the ini-variables-fallback branch from 529ff3c to 4a5eaac Compare May 31, 2023 00:06
@iluuu1994 iluuu1994 added the RFC label May 31, 2023
@iluuu1994
Copy link
Member

@Misterio77 Thank you for your PR! You might get away with just a mail to the mailing list, if nobody objects to the change. If there are discussions on semantics or the usefulness then an RFC is definitely in order.

@Misterio77 Misterio77 force-pushed the ini-variables-fallback branch 10 times, most recently from 95b6c39 to 735ff4a Compare June 3, 2023 00:17
@Misterio77
Copy link
Contributor Author

Misterio77 commented Jun 3, 2023

I redid this and now fallback is a var_string_list instead of a TC_RAW. That means the fallback value behaves similarly to a offset value.

You can do some cool stuff such as nesting:

host = ${HOSTNAME:-localhost:${PORT:-8080}}

If you're using the typed parser, numbers become numbers, as expected:

foo = ${undefined_variable:-12345}
bar = 12345

Both are "int(12345)"

For now, the separator is !, I'd like to make it be :-, but it's a little hard. I'll mark this PR as draft while I work on it. Thanks to @iluuu1994, :- works now.

@Misterio77 Misterio77 marked this pull request as draft June 3, 2023 00:18
@Misterio77 Misterio77 force-pushed the ini-variables-fallback branch from 735ff4a to d659c90 Compare June 3, 2023 00:23
@iluuu1994
Copy link
Member

@Misterio77 I pushed the changes for making :- work, I hope that's ok. Feel free to adjust as you feel appropriate. The next step would be to discuss this change on the mailing list.

@Misterio77
Copy link
Contributor Author

Thanks a lot for helping out, @iluuu1994! I will send an email to the list shortly

@Misterio77 Misterio77 marked this pull request as ready for review June 6, 2023 20:06
@derickr
Copy link
Member

derickr commented Jul 10, 2023

Did you announce this on the ML? I wasn't aware of this... and spend some time on #11661 which is a much simpler solution (I guess, with some different drawbacks).

@derickr
Copy link
Member

derickr commented Jul 13, 2023

Sent an email myself now: https://news-web.php.net/php.internals/120805

@Misterio77
Copy link
Contributor Author

Misterio77 commented Jul 13, 2023

Hey @derickr,

Life got in the way and I couldn't quite muster the energy to write the email. Thanks a lot for sending it! :)

@brzuchal
Copy link
Contributor

Would it make sense to improve php.ini-production and php.ini-development for most/all distributed settings by default?
Meaning entries like this:

; Maximum amount of memory a script may consume
; https://php.net/memory-limit
memory_limit = 128M

Would be extended with env var clause and default to default values, like:

; Maximum amount of memory a script may consume
; https://php.net/memory-limit
memory_limit = ${PHP_MEMORY_LIMIT:-128M}

@Misterio77
Copy link
Contributor Author

Hey @brzuchal, I think it does make sense! Maybe as a separate PR after we merge this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants