Skip to content
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

Cannot set mail.force_extra_parameters as PHP_INI_PERDIR #1081

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

marcovtwout
Copy link
Contributor

Documentation does not match: https://github.com/php/doc-en/blob/3cdd39bb9505e6735d802da83a04870cfa8f2311/appendices/ini.list.xml

This param cannot be set through PHP_INI_PERDIR because of https://bugs.php.net/bug.php?id=71901

@@ -31,7 +31,7 @@
<row>
<entry><link linkend="ini.mail.force_extra_parameters">mail.force_extra_parameters</link></entry>
<entry>NULL</entry>
<entry>PHP_INI_SYSTEM|PHP_INI_PERDIR</entry>
<entry>&php.ini; only</entry>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a tricky one, since the setting can be set in httpd.conf as php_value what is usually only allowed for PHP_INI_PERDIR and PHP_INI_ALL. Maybe leave this line as is:

Suggested change
<entry>&php.ini; only</entry>
<entry>PHP_INI_SYSTEM|PHP_INI_PERDIR</entry>

And change the following line:

Suggested change
<entry>&php.ini; only</entry>
<entry>Cannot be changed in <filename>.htaccess</filename> and <filename>.user.ini</filename></entry>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmb69 If httpd.conf is also allowed that means that https://github.com/php/doc-en/blob/3cdd39bb9505e6735d802da83a04870cfa8f2311/appendices/ini.list.xml is also incorrect.

And is PHP_INI_PERDIR minus .user.ini and .htaccess not effectively the same as PHP_INI_SYSTEM? (according to the options on https://www.php.net/manual/en/configuration.changes.modes.php)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHP_INI_SYSTEM only allows to set the value via php_admin_value, but mail.force_extra_parameters can be set via php_value in .httpconf. Frankly, I wonder why that setting wasn't simply changed to PHP_INI_SYSTEM, but we have to document the status quo.

Copy link
Contributor Author

@marcovtwout marcovtwout Jan 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmb69 Are you absolutely sure mail.force_extra_parameters can be set through the httpd config, and is not being reverted by OnChangeMailForceExtra specified in https://github.com/php/php-src/blob/PHP-7.1.1/main/main.c#L601 ?

Whatever the documented outcome should be, it must be applied to both files (ini.list.xml and ini.xml)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OnChangeMailForceExtra is only there to prevent modifications in .htaccess and .user.ini. Setting mail.force_extra_parameters has effect in .htconf via php_value (changes the local value; master value remains, as usual).

Whatever the documented outcome should be, it must be applied to both files (ini.list.xml and ini.xml)

Indeed, although we should try to get rid of the duplication. If we won't care about the alphabetic order of the directives, the solution would be trivial. Otherwise, this is more ugly/brittle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly this setting can be set in httpd-config through only with php_value and not with php_admin_value? Or can it be set either way?

Both documentation pages only allow for a very short description under column "Changeable". Therefor I would vote to document this in the most concise and clear way possible.

Some suggestions:

  • PHP_INI_SYSTEM (if detail can be ommitted)
  • PHP_INI_SYSTEM with php_value
  • php.ini, or httpd.conf with php_value

Technical nuances could also be added under https://www.php.net/manual/en/mail.configuration.php#ini.mail.force_extra_parameters

@afilina
Copy link
Contributor

afilina commented Dec 23, 2022

Pinging author: @marcovtwout
From my understanding, the reviewer's recommendations would make this PR acceptable.

Co-authored-by: Christoph M. Becker <[email protected]>
@cmb69
Copy link
Member

cmb69 commented Jan 2, 2023

Hmm, as it is now, the PR would not change anything. Are you still working on that @marcovtwout? :)

@afilina
Copy link
Contributor

afilina commented Aug 2, 2023

@cmb69 There has been no activity and it's still a zero-line PR. Does it make sense to close it?

@marcovtwout
Copy link
Contributor Author

marcovtwout commented Aug 3, 2023

@cmb69 I think we were almost there, we just have to pick up the discussion where we left off in the conversation above #1081 (comment)

If you could answer my remaining question there, I'll update the PR with one of the suggested solutions.

@Girgias
Copy link
Member

Girgias commented Aug 3, 2023

Make it PHP_INI_SYSTEM and add the full explanation in the descriptions, as this is probably something we should change/deprecate to make it just PHP_INI_SYSTEM in a future version (would maybe require an RFC)

@afilina afilina closed this Aug 3, 2023
@afilina afilina reopened this Aug 3, 2023
@afilina
Copy link
Contributor

afilina commented Aug 3, 2023

Closing and reopening to see what the pipeline is complaining about. Logs were too old to view.

cannot set mail.force_extra_parameters as PHP_INI_PERDIR
Synchronise change from reference/mail/ini.xml
@marcovtwout marcovtwout requested a review from cmb69 August 4, 2023 07:34
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has no description of the nuance Christopher has said.

This should be documented in the mail/ini.xml file, under the relevant INI description section.

@Girgias Girgias removed the request for review from cmb69 August 4, 2023 15:42
@marcovtwout
Copy link
Contributor Author

@Girgias You're right it doesn't, because I'm not entire sure what to put there exactly. My previous question is still awaiting reply:

If I understand correctly this setting can be set in httpd-config through only with php_value and not with php_admin_value? Or can it be set either way?

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