[4.2] Change Codestyle to PSR-12 #37686

HLeithner · 2022-04-28T15:22:37Z

Pull Request is based on #37681 and continues the work on the PSR-12 move.

This PR introduces a convert script from Joomla coding standard to PSR-12 coding standard. It also converts most of the Joomla! CMS code base to PSR-12.

Motion PROD2021/004: Update the code style to PSR12 as early as possible but latest for 5.0

Motivation

The change of the code style is a long standing issue. Mainly because the last upgrade of the code style is still not completed. On the other side it doesn't make sense to maintain our own code style, mainly because it needs resources to keep the code style up to date to PHP features. So the easiest way is to take a well known code style and move the CMS (and all other Joomla source codes) to this. PSR-12 is maybe not perfect and not everyone likes every part of it but it's the created by the PHP-FIG where Joomla! is part of, so it's obvious to take the "own" and most used one.

Summary of Changes

Remove the Joomla CMS Code style
Remove the Joomla Code style
Add the conversation script
Run code style fixer
Automatically fix some issues which can't be done by phpcbf
- Remove defined('*') or die where it's forbidden and doesn't make sense
- Add exception for missing namespaces
- Add exception for NotCamelCaps in localise.php and recaptcha_invisible plugin
- Add public to all constants defined in a class which don't have a visibility set
- Move comments between } and else { into the else part
Create set line length to 230 (this is a temporary thing mostly because psr12 uses space and not tabs)
Add a list of exception for rules mostly for underscore functions and properties

The new ruleset.xml can be found at it's temporary location build/psr12/ruleset.xml in the same directory are the convert und cleanup script.

Testing Instructions

Code review (which is hard with more then 1800 file changes)
Install Joomla and use it as normal
Upgrade an existing test installation

Actual result BEFORE applying this Pull Request

Works

Expected result AFTER applying this Pull Request

Still works

Documentation Changes Required

Docu about the CMS code style

Additional tasks

Sunset the Joomla code style and the Joomla cms code style

Existing PRs

I'm searching for a way to make the transition so painless as possible.
It's maybe possible to run the script before on the own branch, maybe if possible I create PR against each PR automatically which solves the merge conflicts by updating the changed files. Ideas are welcomed.

Annotation for Testers

Do not create a PR against this PR
Do not expect a linear PR (this PR will be updated with force push)
Please make comments on changes and not suggestions (since the PR will be regenerated base on other merges)

brianteeman · 2022-04-28T16:00:34Z

Firstly let me say - AWESOME - this is long overdue. Is the plan once the inital big changes have been done to add automated testing with fixes on all future pr. It will make life so much easier.

HLeithner · 2022-04-28T16:05:26Z

Is the plan once the inital big changes have been done to add automated testing with fixes on all future pr. It will make life so much easier.

If I figure out how to do it I would say yes. (hound wasn't too successful ;-)

/fyi @nikosdion @PhilETaylor if you like to have a look if I broke something completely (at least the tests are working)

brianteeman · 2022-04-28T16:18:51Z

Would it be possible to change this into two seperate pr. One for the changed files and one for the build/psr12 scripts

I suspect its the scripts only that need commenting on (I know I have a few to make) but its impossible to do that here oin github as there are so many files it crashes the browser.

brianteeman · 2022-04-28T16:29:16Z

build\psr12.editorconfig

is this an intentional commit as we already have an almost identical one in the root of the filesystem and having two both with root=true that are not the same can cause conflicts

nikosdion · 2022-04-28T16:29:52Z

I am sad to see the completely unreadable Egyptian braces being used in control structures. I moved out of that particular code style 12 years ago and reduced the number of my bugs by half. The biggest problem is the } else { is too easy to miss. Anyway, it is what it is, I can always reformat the code using phpStorm on my local repo to make it readable when studying it.

Regarding this PR, um, there are 1809 files. Trying to find what you added among all these files is the very definition to a needle in a haystack. Can you please point out what you added so we can figure out what we are seeing?

brianteeman · 2022-04-28T16:31:26Z

Can you please point out what you added so we can figure out what we are seeing?

it looks like its the build\psr12\ folder

HLeithner · 2022-04-28T16:43:35Z

@nikosdion

Regarding this PR, um, there are 1809 files. Trying to find what you added among all these files is the very definition to a needle in a haystack. Can you please point out what you added so we can figure out what we are seeing?

As brian sad I added the files in build/psr12 that's the scripts that do the complete conversation. It should be enough to run only convert_psr12.php. in the script you have an option array at the beginning

$tasks = [
    'CBF' => true,
    'CLEAN'  => true,
    'CS'  => true,
];

first run (disable clean)

$tasks = [
    'CBF' => true,
    'CLEAN'  => false,
    'CS'  => true,
];

then commit
then run
clean_errors.php
then run
convert_psr12.php
again with CLEAN still disabled. then you only have the changes done by my programming and not by phpcbf (I would expect that's working)

@brianteeman I will move the PSR12 directory to the other pr #37681 on the next run.

You are right the .editorconfig have to be moved to the root folder (it changes the indent_style to space and the indent_size to 4)

brianteeman · 2022-04-28T17:02:35Z

not a massive fan of some of the changes but I am a fan of standardisation.

The one that really concerns me is the removal of the jexec or die check which appears to be in conflict with #PROD2020/023 - Policy for Full Path Disclosure

brianteeman · 2022-04-28T17:19:28Z

C:\htdocs\joomla-cms>php build/psr12/convert_psr12.php                                                     
PHP Notice:  Undefined variable: checkPath in C:\htdocs\joomla-cms\build\psr12\convert_psr12.php on line 71
Fix C:\htdocs\joomla-cms/plugins/



No fixable errors were found

Time: 95ms; Memory: 10MB

Check C:\htdocs\joomla-cms/plugins/
PHP Warning:  file_put_contents(C:\htdocs\joomla-cms\build\psr12/../tmp/psr12/cleanup.json): failed to open stream: No such file or directory in C:\htdocs\joomla-cms\build\psr12\phpcs.joomla.report.php on line 239
<?xml version="1.0" encoding="UTF-8"?>
<phpcs version="3.6.2">
</phpcs>
Fix C:\htdocs\joomla-cms/plugins/

😢

HLeithner · 2022-04-28T17:26:44Z

C:\htdocs\joomla-cms>php build/psr12/convert_psr12.php                                                     
PHP Notice:  Undefined variable: checkPath in C:\htdocs\joomla-cms\build\psr12\convert_psr12.php on line 71
Fix C:\htdocs\joomla-cms/plugins/



No fixable errors were found

Time: 95ms; Memory: 10MB

Check C:\htdocs\joomla-cms/plugins/
PHP Warning:  file_put_contents(C:\htdocs\joomla-cms\build\psr12/../tmp/psr12/cleanup.json): failed to open stream: No such file or directory in C:\htdocs\joomla-cms\build\psr12\phpcs.joomla.report.php on line 239
<?xml version="1.0" encoding="UTF-8"?>
<phpcs version="3.6.2">
</phpcs>
Fix C:\htdocs\joomla-cms/plugins/

😢

strange please create the directory build/tmp/psr12 ... it should be created automatically but seems to fail... oh it could be because the build/tmp directory doesn't exists...

brianteeman · 2022-04-28T17:28:04Z

strange please create the directory build/tmp/psr12 ... it should be created automatically but seems to fail... oh it could be because the build/tmp directory doesn't exists..

Thats what I was just checking and shouldnt it be ROOT/tmp/psr12 ?

HLeithner · 2022-04-28T17:28:39Z

strange please create the directory build/tmp/psr12 ... it should be created automatically but seems to fail... oh it could be because the build/tmp directory doesn't exists..

Thats what I was just checking and shouldnt it be ROOT/tmp/psr12 ?

no in the build directory

brianteeman · 2022-04-28T17:30:18Z

ok well just tested by manually creating the folder build/tmp/psr12 and that gets rid of PHP Warning: file_put_contents

just need to get rid of the php notice on checkpath

HLeithner · 2022-04-28T17:30:44Z

not a massive fan of some of the changes but I am a fan of standardisation.

The one that really concerns me is the removal of the jexec or die check which appears to be in conflict with #PROD2020/023 - Policy for Full Path Disclosure

That doesn't effect this because the style checker doesn't allow executable code in the same file as a symbol is defined. So we remove defined or die only in files that have only classes defined. In this there will be no code executed so we don't have a path disclosure.

brianteeman · 2022-04-28T17:31:38Z

That doesn't effect this because the style checker doesn't allow executable code in the same file as a symbol is defined. So we remove defined or die only in files that have only classes defined. In this there will be no code executed so we don't have a path disclosure.

ok cool. As you know its hard to check properly here on github the exact contents of the file

HLeithner · 2022-04-28T17:32:30Z

ok well just tested by manually creating the folder build/tmp/psr12 and that gets rid of PHP Warning: file_put_contents

just need to get rid of the php notice on checkpath

the checkpath just not initialized, actually I didn't tested this code path it should allow us to check only one file/path (comma separated multiple pathes). I fixed that in my local branch already. (just add a $checkPath = false; into the 8th line)

HLeithner · 2022-04-28T17:33:18Z

That doesn't effect this because the style checker doesn't allow executable code in the same file as a symbol is defined. So we remove defined or die only in files that have only classes defined. In this there will be no code executed so we don't have a path disclosure.

ok cool. As you know its hard to check properly here on github the exact contents of the file

That's true so it's better to check the PR out in phpstorm (or what ever you use) and check it locally.

brianteeman · 2022-04-28T17:34:13Z

That's true so it's better to check the PR out in phpstorm (or what ever you use) and check it locally.

Which I did although even vscode balked at the number of files in review mode

brianteeman · 2022-04-28T17:39:56Z

Conversation complete please complete the following manual tasks:
=================================================================

 * Update .drone.yml to use PSR12 or CMS coding standard 3.0 which ever suits after this script is complete

Correct first word and simplify drone comment. no need to say after complete as its only displayed then.


Conversion complete please complete the following manual tasks:
=================================================================

 * Update .drone.yml to use PSR12 or CMS coding standard 3.0

brianteeman · 2022-04-28T17:42:46Z

php build/psr12/convert_psr12.php

I assume that this should be checking all the folders in $baseFolders

it is not. It is only checking the last entry

ReLater · 2022-04-28T23:57:20Z

Maybe I'm wrong?
Do I see that right? Really? 4 spaces instead of a single tab? Makes the codes just more unreadable (overlong lines) and inflexible (concerning tab width configuration in editors) and harder to type. In my opinion, you don't have to run after every standard just because someone calls it a standard.

brianteeman · 2022-04-29T05:47:22Z

@ReLater I hear you but as long as corrections are dont automatically I dont care

HLeithner · 2022-05-11T07:15:40Z

@nikosdion I added you readonly to the repo that should work for codeowners. At least that's writte in https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners
The people you choose as code owners must have read permissions for the repository

But the validation on the codeowners file fails... so it seems still not possible to add someone as codeowner without write permissions.

nikosdion · 2022-05-11T07:23:18Z

@HLeithner Ah, bugger. I guess trying to implement a notification (like at-tagging us in the comments) in the automation that assigns labels to PRs is a tall ask, right?

I have no idea how that automation works to be honest, I don't even know where its code is, I am just thinking out loud.

nikosdion · 2022-05-11T07:24:00Z

Wait, I realised that you commented before I accepted the invite to the repo. Can you double check that the validation still fails?

HLeithner · 2022-05-11T07:26:49Z

@nikosdion

Wait, I realised that you commented before I accepted the invite to the repo. Can you double check that the validation still fails?

I checked it with hannes who also as no write permssion to the repo but was in the organisation.
But I double checked it now and still got the same error:

HLeithner · 2022-05-11T07:28:47Z

@HLeithner Ah, bugger. I guess trying to implement a notification (like at-tagging us in the comments) in the automation that assigns labels to PRs is a tall ask, right?

I have no idea how that automation works to be honest, I don't even know where its code is, I am just thinking out loud.

This is done in jissues (at least I think it is...) https://github.com/joomla/jissues tbh I never touched this and I personally would like to get rid because no one once to maintain it... but some people like it because it has some features which github doesn't have....

nikosdion · 2022-05-11T08:25:32Z

Yeah, I never understood why we have an issue tracker which is simply an interface to GitHub. The biggest pain for me is when I test a PR I have to go to that site to log my test result otherwise it does not appear on GitHub. I understand the context it was created in — back then GitHub's search was infantile at best. It's just there's no longer a need for it; GitHub can do all the important things and has a powerful search which actually works!

brianteeman · 2022-05-11T08:43:31Z

. but some people like it because it has some features which github doesn't have..

While definitely true when it was created I would be interested to know what features are still missing. More than likely they do exist on github we just havent implemented them

HLeithner · 2022-05-12T13:27:32Z

. but some people like it because it has some features which github doesn't have..

While definitely true when it was created I would be interested to know what features are still missing. More than likely they do exist on github we just havent implemented them

at least 2, tests marking and it seems the Categories are used but that's off-topic here

brianteeman · 2022-06-18T21:00:15Z

Does this mean that this is now postponed until 4.3?

HLeithner · 2022-06-19T07:11:41Z

Does this mean that this is now postponed until 4.3?

not necessary because actually no code change ;-)

sadly I'm busy at the moment with other Joomla stuff and paid work, so this 2 prs have to wait a bit

HLeithner requested review from bembelimen, zero-24, rdeutz and laoneo as code owners April 28, 2022 15:22

joomla-cms-bot added PR-4.2-dev Unit/System Tests labels Apr 28, 2022

HLeithner marked this pull request as draft April 28, 2022 15:24

zero-24 removed their request for review April 28, 2022 15:26

HLeithner force-pushed the 4.2-psr12 branch from 0c43123 to a342543 Compare May 18, 2022 07:41

HLeithner force-pushed the 4.2-psr12 branch 4 times, most recently from f2ef82d to 8d7ccf4 Compare June 27, 2022 18:10

HLeithner closed this Jun 27, 2022

HLeithner force-pushed the 4.2-psr12 branch from 8d7ccf4 to 6d9cc0f Compare June 27, 2022 18:13

HLeithner added 3 commits June 27, 2022 20:18

Phase 1 convert CMS to PSR-12

e7f5cc1

Phase 2 convert CMS to PSR-12

dd91072

PSR12

6a01d92

HLeithner reopened this Jun 27, 2022

HLeithner marked this pull request as ready for review June 27, 2022 18:23

HLeithner requested review from chmst and Hackwar as code owners June 27, 2022 18:23

HLeithner merged commit 09d14c6 into joomla:4.2-dev Jun 27, 2022

richard67 added this to the Joomla! 4.2.0 milestone Jun 30, 2022

sandewt mentioned this pull request Jul 10, 2022

[4.x] SEF, Implement Case insensitive url, but only for path part of URL #38249

Closed

brianteeman mentioned this pull request Aug 27, 2022

[4.2][Security] Re-add jexec or die #38615

Merged

heelc29 mentioned this pull request Nov 12, 2022

[4.3] Convert missing files to PSR-12 code style #39197

Merged

4 tasks

Quy mentioned this pull request Mar 19, 2023

Style #40153

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4.2] Change Codestyle to PSR-12 #37686

[4.2] Change Codestyle to PSR-12 #37686

HLeithner commented Apr 28, 2022 •

edited

brianteeman commented Apr 28, 2022

HLeithner commented Apr 28, 2022

brianteeman commented Apr 28, 2022

brianteeman commented Apr 28, 2022

nikosdion commented Apr 28, 2022

brianteeman commented Apr 28, 2022 •

edited

HLeithner commented Apr 28, 2022

brianteeman commented Apr 28, 2022

brianteeman commented Apr 28, 2022

HLeithner commented Apr 28, 2022

brianteeman commented Apr 28, 2022

HLeithner commented Apr 28, 2022

brianteeman commented Apr 28, 2022

HLeithner commented Apr 28, 2022

brianteeman commented Apr 28, 2022

HLeithner commented Apr 28, 2022

HLeithner commented Apr 28, 2022

brianteeman commented Apr 28, 2022

brianteeman commented Apr 28, 2022

brianteeman commented Apr 28, 2022

ReLater commented Apr 28, 2022

brianteeman commented Apr 29, 2022

HLeithner commented May 11, 2022

nikosdion commented May 11, 2022

nikosdion commented May 11, 2022

HLeithner commented May 11, 2022

HLeithner commented May 11, 2022

nikosdion commented May 11, 2022

brianteeman commented May 11, 2022

HLeithner commented May 12, 2022

brianteeman commented Jun 18, 2022

HLeithner commented Jun 19, 2022

[4.2] Change Codestyle to PSR-12 #37686

[4.2] Change Codestyle to PSR-12 #37686

Conversation

HLeithner commented Apr 28, 2022 • edited

Motivation

Summary of Changes

Testing Instructions

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Documentation Changes Required

Additional tasks

Existing PRs

Annotation for Testers

brianteeman commented Apr 28, 2022

HLeithner commented Apr 28, 2022

brianteeman commented Apr 28, 2022

brianteeman commented Apr 28, 2022

nikosdion commented Apr 28, 2022

brianteeman commented Apr 28, 2022 • edited

HLeithner commented Apr 28, 2022

brianteeman commented Apr 28, 2022

brianteeman commented Apr 28, 2022

HLeithner commented Apr 28, 2022

brianteeman commented Apr 28, 2022

HLeithner commented Apr 28, 2022

brianteeman commented Apr 28, 2022

HLeithner commented Apr 28, 2022

brianteeman commented Apr 28, 2022

HLeithner commented Apr 28, 2022

HLeithner commented Apr 28, 2022

brianteeman commented Apr 28, 2022

brianteeman commented Apr 28, 2022

brianteeman commented Apr 28, 2022

ReLater commented Apr 28, 2022

brianteeman commented Apr 29, 2022

HLeithner commented May 11, 2022

nikosdion commented May 11, 2022

nikosdion commented May 11, 2022

HLeithner commented May 11, 2022

HLeithner commented May 11, 2022

nikosdion commented May 11, 2022

brianteeman commented May 11, 2022

HLeithner commented May 12, 2022

brianteeman commented Jun 18, 2022

HLeithner commented Jun 19, 2022

HLeithner commented Apr 28, 2022 •

edited

brianteeman commented Apr 28, 2022 •

edited