[#38167] - Change PHP code style to PSR-12 for the build folder
- Fixed in Code Base
- 28 Jun 2022
- Medium
- Build: 4.2-dev
- # 38167
- Diff
- richard67:4.2-dev-psr12-code-style-for-build-scripts
User tests: Successful: Unsuccessful:
Pull Request for Issue # .
Summary of Changes
Converts the PHP files in the build folder to PSR-12 and but does not include them into the cs check yet.
The files in subfolder "psr12" are already PSR-12, but this PR fixes a few smaller code style errors in these files, too.
This PR here makes sure that people who use the right .editorconfig for PSR-12 can edit the files in the build folder without getting mad.
Testing Instructions
On a clean, current 4.2-dev run composer install if not done before.
Then run ./libraries/vendor/bin/phpcs --extensions=php -p --standard=ruleset.xml ./build
Actual result BEFORE applying this Pull Request
Lots of code style errors and warnings.
Expected result AFTER applying this Pull Request
Only the following warning is remaining for some files:
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------------------
1 | WARNING | A file should declare new symbols (classes, functions, constants, etc.) and cause no other side effects, or it should execute logic
| | with side effects, but should not do both. The first symbol is defined on line 25 and the first side effect is on line 26.
------------------------------------------------------------------------------------------------------------------------------------------------------
Documentation Changes Required
None.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Repository |
| Status | Pending | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-06-28 11:32:58 |
| Closed_By | ⇒ | HLeithner | |
| Labels |
Added:
?
|
||
You can add there like what I did in https://github.com/joomla/joomla-cms/blob/4.2-dev/tests/Unit/bootstrap.php#L13
I would also add it to the CS check as this should be for all files.
When we do exceptions I think we should describe the reason for the code style exception, especially for then PSR1.Files.SideEffects which has security implications.
When we do exceptions I think we should describe the reason for the code style exception, especially for then
PSR1.Files.SideEffectswhich has security implications.
@HLeithner For the scripts in the build folder we can add a comment that they are not shipped and so not relevant.
Same for tests
I think we should not do that in particular files but have exclusion sets for that rule in the ruleset.xml for the build and the tests folder.
I would not do that for a particular folder, more for the individual files. For the build and tests folder should also go under the same standard as the rest of the code base.
I would not do that for a particular folder, more for the individual files. For the build and tests folder should also go under the same standard as the rest of the code base.
@laoneo Then why have you added an exception for a folder here with PR #38161 ? https://github.com/joomla/joomla-cms/blob/4.2-dev/ruleset.xml#L247
In the end we should have "no" exceptions, of course that's not possible but I'm pretty sure we now have a punch of cs ignore rules in our codebase which could be removed after the move to the psr-12 standard.
When someone has time we have to reduce the char count to about 150 but that wasn't possible with our current code base so I set the longest line I found.
I think we should do that for particular rules in the ruleset.xml because there are already comments for particular sections, like <!-- expected, may move exception to file or refactor implementation -->. I will make my PR like that.
what pr would you like to create?!
The one to include the build folder into the regular tests, i.e. remove it from the exception in ruleset.xml
@HLeithner See #38168 .
thanks