Cosmetic changes inside php.ini by diceydust · Pull Request #17282 · php/php-src

diceydust · 2024-12-27T13:46:50Z

This PR fixes #17266. It does some cosmetic changes to php.ini file (dev and prod). Currently inside php.ini there are lot of inconsistencies. This is an attempt to fix those. Main changes are:

notation of PHP directives is standarized according to the following rules:

directive names are all-lowercase (which is why SMTP inside mail function is now smtp),
equals signs are surrounded by spaces,
string values are surrounded by double quotation marks (even if they are empty, i.e. url_rewriter.hosts = ""),
boolean values are written as On/Off,

all section headers inside [] and ;; are written in all-lowercase convention,
double spaces between sentences inside block comments are converted into single spaces.

This reverts commit a5d10b5.

Girgias

I'm not sure all the cosmetic changes make sense.

Girgias · 2024-12-27T14:28:55Z

+; Set to 1 if running under IIS. Default is zero.
 ; https://php.net/fastcgi.impersonate
-;fastcgi.impersonate = 1
+;fastcgi.impersonate = "1"


Is this a boolean INI setting or not? If not it may make sense to convert it to one.

According to the documentation this is string. Once it becomes bool I will fix this in the future PR.

The documentation, especially for INI settings, is not the source of truth. The source code is.

OK, thanks for the input. I'm gonna look up all of the directives inside the source code.

php-src/sapi/cgi/cgi_main.c

Line 1516 in f90323c

STD_PHP_INI_BOOLEAN("fastcgi.impersonate", "0", PHP_INI_SYSTEM, OnUpdateBool, impersonate, php_cgi_globals_struct, php_cgi_globals)

Girgias · 2024-12-27T14:34:41Z


 ;;;;;;;;;;;;;;;;;;;;;;
-; Dynamic Extensions ;
+; dynamic extensions ;


I don't get these changes, this is using Title Case for "section" titles, which seems adequate?

Yeah, I get the point. However in the curent master there is:

; About this file ; (First letter of the first word is uppercase)

; Quick Reference ; (First letter of each word is uppercase)

; php.ini Options ; (First letter of the second word is uppercase – yes, I know php.ini is a file name)

What's the right convention then?

Girgias · 2024-12-27T14:35:07Z

 ;;;;;;;;;;;;;;;;;;;

-[CLI Server]
+[cli server]


Ditto, these are "section" titles

Again, in the current master we've got:

[intl]
[filter]
[mail function] – all lowercase
[Session] – first letter uppercase
[ODBC] – all caps
[MySQLi] but [mysqlnd]
[Pcre]
[bcmath]

Isn't it much simpler to go with lowercase all the way?

These are extension names so these are proper nouns. Having said that, some of them look incorrect.

https://www.pcre.org/ should be PCRE

https://www.php.net/manual/en/book.bc.php & https://github.com/php/php-src/blob/master/ext/bcmath/CREDITS should be BC Math

filter should probably be Filter https://www.php.net/manual/en/book.filter.php

Girgias · 2024-12-27T14:36:40Z

-; Maximum number of links (persistent + non-persistent).  -1 means no limit.
+; Maximum number of links (persistent + non-persistent). -1 means no limit.


Double space after a FULL STOP is a convention for English, it is not mandatory but I'm not sure I see the problem with it.

In the php.ini of the current master there are 169 situations where one sentence is followed by another one in the same line. In 44 cases they are separated by double space, in other 125 cases single space is used. Which is why I went for single space.

I don't know if this is worth standardizing. A lot of people (especially older generations) prefer double spaces. You're just going to start a debate on which way is more correct.

Girgias · 2024-12-27T14:37:24Z

-;mysqli.allow_local_infile = On
+;mysqli.allow_local_infile = 1


Does this have more options than On/Off?

According to the documentation this is int.

Ditto, documentation is not the source of truth.

php-src/ext/mysqli/mysqli.c

Line 443 in f90323c

STD_PHP_INI_BOOLEAN("mysqli.allow_local_infile", "0", PHP_INI_SYSTEM, OnUpdateBool, allow_local_infile, zend_mysqli_globals, mysqli_globals)

Girgias · 2024-12-27T14:39:27Z

-mysqli.allow_persistent = On
+mysqli.allow_persistent = 1


Int again: documentation.

php-src/ext/mysqli/mysqli.c

Line 432 in f90323c

STD_PHP_INI_BOOLEAN("mysqli.allow_persistent", "1", PHP_INI_SYSTEM, OnUpdateBool, allow_persistent, zend_mysqli_globals, mysqli_globals)

Girgias · 2024-12-27T14:39:41Z

 ; Detect broken persistent links always with pg_pconnect().
 ; Auto reset feature requires a little overheads.
 ; https://php.net/pgsql.auto-reset-persistent
-pgsql.auto_reset_persistent = Off


Int: documentation.

Girgias · 2024-12-27T14:40:01Z

 ; Log PostgreSQL backends Notice message or not.
-; Unless pgsql.ignore_notice=0, module cannot log notice message.
+; Unless pgsql.ignore_notice = 0, module cannot log notice message.
 ; https://php.net/pgsql.log-notice
 pgsql.log_notice = 0


Boolean INI setting?

According to the documentation this is int.

thg2k · 2024-12-27T16:32:09Z

I tried something similar few years ago in #7643 but was not successful. I still think a cleanup is due on these files but I don't like the changes in this particular PR.

diceydust · 2024-12-27T18:05:35Z

I tried something similar few years ago in #7643 but was not successful. I still think a cleanup is due on these files but I don't like the changes in this particular PR.

Yeah, these things are difficult to standarize as – for instance – half of the people prefer double space between sentences and the other half – single space.

But the current situation is that there are multiple conventions used inside a single file, which I belive is even worse than having a bad standarization.

kamil-tekiela · 2024-12-27T19:54:51Z

I would recommend splitting this into several PRs. There are several changes here that I would welcome but many that I disagree with. It would also be easier to review. For example, "equals signs are surrounded by spaces," could be a PR on its own.

diceydust · 2024-12-28T14:36:51Z

I went through all all of the directives in the source code to check their data types. One thing I noticed is that I couldn't find declarations of the following ones:

url_rewriter.tags,
url_rewriter.hosts,
session.trans_sid_tags,
session.trans_sid_hosts.

Are they still part of PHP?

devnexen · 2024-12-28T14:49:51Z

        STD_PHP_INI_ENTRY("session.trans_sid_tags", "a=href,area=href,frame=src,form=", PHP_INI_ALL, OnUpdateSessionTags, url_adapt_session_ex, php_basic_globals, basic_globals)
	STD_PHP_INI_ENTRY("session.trans_sid_hosts", "", PHP_INI_ALL, OnUpdateSessionHosts, url_adapt_session_hosts_ht, php_basic_globals, basic_globals)
	STD_PHP_INI_ENTRY("url_rewriter.tags", "form=", PHP_INI_ALL, OnUpdateOutputTags, url_adapt_session_ex, php_basic_globals, basic_globals)
	STD_PHP_INI_ENTRY("url_rewriter.hosts", "", PHP_INI_ALL, OnUpdateOutputHosts, url_adapt_session_hosts_ht, php_basic_globals, basic_globals)

I believe the two firsts are deprecated tough..

diceydust · 2024-12-30T23:10:50Z

OK, so I'm gonna take step-by-step approach in tidying up php.ini, As @kamil-tekiela suggested. Therefore I'm closing this one.

diceydust added 3 commits December 27, 2024 01:54

Cosmetic changes in php.ini

a5d10b5

Revert "Cosmetic changes in php.ini"

0b9ed2d

This reverts commit a5d10b5.

Cosmetic changes inside php.ini

337f611

devnexen requested a review from Girgias December 27, 2024 13:59

diceydust mentioned this pull request Dec 27, 2024

php.ini directives – attempt to standarize notation #17266

Open

Girgias reviewed Dec 27, 2024

View reviewed changes

jimwins mentioned this pull request Dec 27, 2024

Clarify type of a few INI values php/doc-en#4347

Merged

diceydust closed this Dec 30, 2024

		; Maximum number of links (persistent + non-persistent). -1 means no limit.
		; Maximum number of links (persistent + non-persistent). -1 means no limit.

		;mysqli.allow_local_infile = On
		;mysqli.allow_local_infile = 1

		mysqli.allow_persistent = On
		mysqli.allow_persistent = 1

Conversation

diceydust commented Dec 27, 2024

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thg2k commented Dec 27, 2024

Uh oh!

diceydust commented Dec 27, 2024

Uh oh!

kamil-tekiela commented Dec 27, 2024

Uh oh!

diceydust commented Dec 28, 2024

Uh oh!

devnexen commented Dec 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

diceydust commented Dec 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

devnexen commented Dec 28, 2024 •

edited

Loading