FSFE/fsfe-website
30
33
Fork 98
Code Issues 66 Pull Requests 19 Wiki Activity

fix/php-cs-fixer #5538

Merged
tobiasd merged 4 commits from fix/php-cs-fixer into master 2025-12-22 14:59:04 +00:00
Conversation 9 Commits 4 Files Changed 14
+783 -729
delliott commented 2025-12-03 11:38:55 +00:00
Member
Copy Link
Copy Source

fixes some issues with the latest version of php-cs-fixer and runs it on all files.

solved the ci hangs we are seeing in #5528 and #5512

fixes some issues with the latest version of php-cs-fixer and runs it on all files. solved the ci hangs we are seeing in https://git.fsfe.org/FSFE/fsfe-website/pulls/5528 and https://git.fsfe.org/FSFE/fsfe-website/pulls/5512
delliott requested review from tobiasd 2025-12-03 11:39:06 +00:00
delliott force-pushed fix/php-cs-fixer from 90f597403b to 5bedb6a391 2025-12-03 11:42:39 +00:00 Compare
delliott force-pushed fix/php-cs-fixer from 5bedb6a391 to a82b2c5276 2025-12-03 12:01:51 +00:00 Compare
tobiasd reviewed 2025-12-04 06:07:32 +00:00
fsfe.org/cgi-bin/promotion.php Outdated
@@ -4,3 +4,2 @@
{
$dir = dirname(__FILE__) . '/../templates';
$result = file_get_contents("$dir/$template");
$dir = dirname(__FILE__).'/../templates';
tobiasd commented 2025-12-04 06:07:32 +00:00
Owner
Copy Link
Copy Source

The spaces around the . had been there for readability of the code. Do they have to be removed to make the linter happy?

The spaces around the `.` had been there for readability of the code. Do they have to be removed to make the linter happy?
delliott commented 2025-12-16 10:25:42 +00:00
Author
Member
Copy Link
Copy Source

It seems so.

It seems so.
tobiasd reviewed 2025-12-04 06:15:27 +00:00
fsfe.org/cgi-bin/promotion.php Outdated
@@ -25,3 +27,3 @@
$alphabet = '0123456789abcdefghijklmnopqrstuvwxyz';
$ret = '';
for ($digits; $digits > 0; $digits--) {
for ($digits; $digits > 0; --$digits) {
tobiasd commented 2025-12-04 06:15:27 +00:00
Owner
Copy Link
Copy Source

Don't we miss one digit here with the change? As the counter is first reduced before checked if it is larger then 0. Could you check that the returned string has the correct length please?

Don't we miss one digit here with the change? As the counter is first reduced before checked if it is larger then 0. Could you check that the returned string has the correct length please?
delliott commented 2025-12-16 10:27:33 +00:00
Author
Member
Copy Link
Copy Source

Some quick testing in a REPL shows that they are equivalent:

❯ php -a
Interactive shell

php > $digits = 10;
php >  for ($digits; $digits > 0; $digits--) {echo $digits; echo " ";};
10 9 8 7 6 5 4 3 2 1
php > $digits = 10;
php >  for ($digits; $digits > 0; --$digits) {echo $digits; echo " ";};
10 9 8 7 6 5 4 3 2 1
php >
Some quick testing in a REPL shows that they are equivalent: ```php ❯ php -a Interactive shell php > $digits = 10; php > for ($digits; $digits > 0; $digits--) {echo $digits; echo " ";}; 10 9 8 7 6 5 4 3 2 1 php > $digits = 10; php > for ($digits; $digits > 0; --$digits) {echo $digits; echo " ";}; 10 9 8 7 6 5 4 3 2 1 php > ```
delliott marked this conversation as resolved
tobiasd reviewed 2025-12-04 06:33:08 +00:00
fsfe.org/cgi-bin/registerevent.php Outdated
@@ -254,3 +255,1 @@
$digit .
"-" .
$eventhash;
for ($count = 1; $count <= 30; ++$count) {
tobiasd commented 2025-12-04 06:33:07 +00:00
Owner
Copy Link
Copy Source

Same for the resulting length of the string here because of the changed handling of the loop counter, though I think it does not matter much in this case.

Same for the resulting length of the string here because of the changed handling of the loop counter, though I think it does not matter much in this case.
delliott commented 2025-12-16 10:29:01 +00:00
Author
Member
Copy Link
Copy Source

Again, REPL testing shows equivelance:

php > for ($count = 1; $count <= 30; ++$count) { echo $count; echo " ";};
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30
php > for ($count = 1; $count <= 30; $count++) { echo $count; echo " ";};
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30
Again, REPL testing shows equivelance: ```php php > for ($count = 1; $count <= 30; ++$count) { echo $count; echo " ";}; 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 php > for ($count = 1; $count <= 30; $count++) { echo $count; echo " ";}; 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 ```
tobiasd reviewed 2025-12-04 06:41:20 +00:00
fsfe.org/cgi-bin/reimbursement.php Outdated
@@ -59,3 +59,3 @@
$filename = preg_replace(
'~
[<>:"/\\|?*]| # file system reserved https://en.wikipedia.org/wiki/Filename#Reserved_characters_and_words
[<>:"/\|?*]| # file system reserved https://en.wikipedia.org/wiki/Filename#Reserved_characters_and_words
tobiasd commented 2025-12-04 06:41:20 +00:00
Owner
Copy Link
Copy Source

Did PHP change behavior here, that the \ does not need to be masked anymore?

Did PHP change behavior here, that the `\` does not need to be masked anymore?
delliott commented 2025-12-16 10:23:50 +00:00
Author
Member
Copy Link
Copy Source

So, I did some testing in a REPL, using php84, the one in use in deployment. And it turns out we never escaped backslashes properly. So php-cs-fixer presumed we were just making a mess of escaping the pipe operator, and streamlined it.

See below for examples:

❯ php -a
Interactive shell

php >  $filename = 'test/file\junk';
php > $filename;
php > echo $filename;
test/file\junk
php > echo preg_replace('~[<>:"/\\|?*]~x', '-', $filename);
test-file\junk
php > echo preg_replace('~[<>:"/\|?*]~x', '-', $filename);
test-file\junk
php > echo preg_replace('~[<>:"/\\\\|?*]~x', '-', $filename);
test-file-junk
php >
So, I did some testing in a REPL, using php84, the one in use in deployment. And it turns out we never escaped backslashes properly. So php-cs-fixer presumed we were just making a mess of escaping the pipe operator, and streamlined it. See below for examples: ```php ❯ php -a Interactive shell php > $filename = 'test/file\junk'; php > $filename; php > echo $filename; test/file\junk php > echo preg_replace('~[<>:"/\\|?*]~x', '-', $filename); test-file\junk php > echo preg_replace('~[<>:"/\|?*]~x', '-', $filename); test-file\junk php > echo preg_replace('~[<>:"/\\\\|?*]~x', '-', $filename); test-file-junk php > ```
delliott marked this conversation as resolved
delliott commented 2025-12-04 09:10:23 +00:00
Author
Member
Copy Link
Copy Source

In response to all comments: I have just let the linter loose and know very little about the changes it is making. I have disabled risky changes and only enabled essentially the default ruleset, so I imagine it should not break things. But perhaps I am too faithful in the quality of it.

Best thing to do is deploy this on test and run through all scripts at some point?

In response to all comments: I have just let the linter loose and know very little about the changes it is making. I have disabled risky changes and only enabled essentially the default ruleset, so I imagine it should not break things. But perhaps I am too faithful in the quality of it. Best thing to do is deploy this on test and run through all scripts at some point?
delliott marked the pull request as work in progress 2025-12-04 09:10:34 +00:00
delliott commented 2025-12-04 09:17:52 +00:00
Author
Member
Copy Link
Copy Source

Seeing as this is likely going to be a longer thing now, I have added the minimal fix --no-interactive to the prs blocked on this, so this is now a separate thing we can solve at leisure.

Seeing as this is likely going to be a longer thing now, I have added the minimal fix `--no-interactive` to the prs blocked on this, so this is now a separate thing we can solve at leisure.
delliott force-pushed fix/php-cs-fixer from a82b2c5276 to 015c335554 2025-12-04 10:25:25 +00:00 Compare
delliott force-pushed fix/php-cs-fixer from 015c335554 to b1c0826ead 2025-12-04 12:18:28 +00:00 Compare
delliott force-pushed fix/php-cs-fixer from b1c0826ead to dd84e65f70 2025-12-04 15:29:08 +00:00 Compare
delliott commented 2025-12-04 15:38:22 +00:00
Author
Member
Copy Link
Copy Source

Going to take a while to land this, so to unblock stuff landed a simpler version that does not impose strict linting:

#5549

Going to take a while to land this, so to unblock stuff landed a simpler version that does not impose strict linting: https://git.fsfe.org/FSFE/fsfe-website/pulls/5549
delliott added the cgi Scripting label 2025-12-14 17:48:23 +00:00
delliott force-pushed fix/php-cs-fixer from dd84e65f70 to 856202a68e 2025-12-16 10:30:32 +00:00 Compare
delliott commented 2025-12-16 10:30:46 +00:00
Author
Member
Copy Link
Copy Source

SO, had a look there and it seems in all cases behavior is the same before and after the lint run.

I have not tested the CGI scripts in deployment, but evaluating expressions in the repl shows the same behavior before and after the lint run.

Seeing as we use only the default ruleset, with all risky rules disabled I think it unlikely this has broken anything.

SO, had a look there and it seems in all cases behavior is the same before and after the lint run. I have not tested the CGI scripts in deployment, but evaluating expressions in the repl shows the same behavior before and after the lint run. Seeing as we use only the default ruleset, with all risky rules disabled I think it unlikely this has broken anything.
delliott marked the pull request as ready for review 2025-12-16 10:30:52 +00:00
delliott commented 2025-12-16 10:31:56 +00:00
Author
Member
Copy Link
Copy Source

SO, had a look there and it seems in all cases behavior is the same before and after the lint run.

I have not tested the CGI scripts in deployment, but evaluating expressions in the repl shows the same behavior before and after the lint run.

Seeing as we use only the default ruleset, with all risky rules disabled I think it unlikely this has broken anything.

But I am not hugely familiar with PHP or php-cs-fixer, so that is not a very weighty opinion.

SO, had a look there and it seems in all cases behavior is the same before and after the lint run. I have not tested the CGI scripts in deployment, but evaluating expressions in the repl shows the same behavior before and after the lint run. Seeing as we use only the default ruleset, with all risky rules disabled I think it unlikely this has broken anything. But I am not hugely familiar with PHP or php-cs-fixer, so that is not a very weighty opinion.
delliott force-pushed fix/php-cs-fixer from 856202a68e to 69e71d571c 2025-12-21 17:03:28 +00:00 Compare
tobiasd merged commit ec321f1f99 into master 2025-12-22 14:59:04 +00:00
tobiasd referenced this issue from a commit 2025-12-22 14:59:04 +00:00
fix/php-cs-fixer (#5538)
tobiasd deleted branch fix/php-cs-fixer 2025-12-22 14:59:05 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
tobiasd
Labels
Clear labels
Accessibility
bug
build
cgi Scripting
design
disruptive
Requires careful coordination and documentation changes
 documentation
duplicate
easy
enhancement
Improving what is already there
 feature-request
help wanted
javascript
priority/low
question
system-hackers
tagging
text
translations
wait/bugfix
wait/inprogress
wait/misc
wait/proofread
wontfix
xsl
No labels cgi Scripting
Milestone
No items
No Milestone
Assignees
Clear assignees
albert (Albert Dengg) alex.busch (alex.busch) alex.sander (alex.sander) anaghz (anaghz) annarita.russo (annarita.russo) ao (André Ockers) bonnie (bonnie) ciampix (Marco Ciampa) cryptie (cryptie) dario (dario) delliott (delliott) dfajfer (Damian Fajfer) dmaphy (Dominic Hopf) doczkal (Thomas Doczkal) eal (eal) egnun (Erik Grun) eventregbot (eventregbot) fi (fi) floriansnow (Florian Snow) gabriel.ku (gabriel.ku) guido (guido) hf (hf) hugo (hugo) ineiev (ineiev) jithendra (jithendra) jn (jn) jzarl (jzarl) linus (Linus Sehn) lucabon (Luca Bonissi) lucas.lasota (lucas.lasota) max.mehl (Max Mehl) mk (Matthias Kirschner) monochromec (monochromec) mweimann (Michael Weimann) nico.rikken (nico.rikken) patrick (Patrick Ohnewein) reinhard (Reinhard Müller) renovate-bot (Renovate Bot) repentinus (Heiki Lõhmus) schiessle (Björn Schießle) silviarbgl (Silvia Rbgl) sofiaritz (Sofía Aritz) tobiasd (tobiasd) vincent (vincent)
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: FSFE/fsfe-website#5538