Been pulling my hair out all day trying to install, configure and use phpcs and php-cs-fixer and then work out why I was getting weird results
It's a rabbit hole of conflicting instructions not least of which is that there are two different standards in two different repos.
The documentation states
the definitive standard for coding in Joomla, whether that is for the core Joomla Framework or an extension that forms part of the stack of the Joomla CMS.
But the only repo referred to there (by link not name) is joomla/coding-standards
However it seems that we need both as they are both listed in composer.json
"joomla/cms-coding-standards": "~2.0.0-alpha2@dev",
"joomla/coding-standards": "~3.0@dev",
But then it turns out that although there is still contribution and updates to both of these standards there have been no new releases for a long time so the rulesets are out of date
joomla/cms-coding-standards 13 Jul 2019
joomla/coding-standards 18 Aug 2020
/me very confused
To make it worse, if I am reading this correctly, then large swathes of the files are excluded from the rulesets so are never to tested to see if they comply to the standards.
/me even more confused.
Thanks to @PhilETaylor for starting me on this trip down the rabbit hole
Labels |
Added:
?
|
Im GUESSING but I would say "joomla/coding-standards" is for "Joomla Generic" aka The framework and "joomla/cms-coding-standards" is for Joomla CMS.
That was my "guess" as well but the quoted documentation says that joomla/coding-standards is for everything
Lots of unfinished commentary in joomla/coding-standards#143
To make it worse, if I am reading this correctly, then large swathes of the files are excluded from the rulesets so are never to tested to see if they comply to the standards.
From memory: When it was introduced, all layout files which contain mixed PHP and HTML stuff (eg those in the "tmpl" folder) got excluded. Because we often use a different codestyle there (eg and
instead of &&
or if (condition) : ... endif
instead of if (condition) { ... }
).
That should at least explain part of the excluded files. 3rd party code certainly is also excluded.
Check the exclude list - a lot bigger than that
Why is Joomla trying to roll its own standards anyway - Just use PSR12 like other professional PHP apps do and be done with it!
use php-cs-fixer to comply with PSR12 and be done with it. Simple. Fast. No need to roll its own.
Joomla could even contribute upstream where help is needed FriendsOfPHP/PHP-CS-Fixer#4502
use php-cs-fixer to comply with PSR12 and be done with it. Simple. Fast. No need to roll its own.
I am saying this for years
It's not so easy as it looks, it will break all PRs. You need to find a good moment to make this change, because you change all files. You need to coordinate this with our CI. And there might be other problems I have forgotton.
All that can be overcome and achieved.
Breaking all PR's is not a bad thing. PRs should not sit open for years anyway - oldest currently is dated 2016 ffs
I've sat there at the back of the production room before and heard all the issues. I think its one of those things that comes under "If I was going there I would not have started from here" but we are here so it will need a hard effort to coordinate the change. @rdeutz is it something those who like me dont understand the details can be shown what to do and we just get lots of hands together to fix the issues?
There are even GitHub actions and bots that can resolve and commit PRs to fix code standards. Symfony has fabbot.io
but other services exist like https://styleci.io
At the end of the day it is a political decision.
We can fix all the issues and it would be something we have to go thru one time, but with the option we don't have to do this again. I have been in this discussion 10 times and we never made the change. A good moment could be before 4.0.0, I will put it on the agenda of the next meeting.
That's exactly why I was looking at this today.
As a start Joomla could publish a new code standard document laying out the new standard from that day forward and then not accept any new PRs that dont meet those standards from that day onwards... pending any huge decision on legacy code, and outstanding PRs and any fully automated solutions.
As long as people are given a rock solid standard, and told to apply that standard, we would! Even kids can follow the school rules!
For example, in the last few days, many PRs have been merged still calling deprecated methods, instead of using the new Joomla 4 methods (from memory Factory::getUser()
is deprecated for example) - Joomla should not still be accepting such code.
@softforge from what I learned today its really just a case of getting the rules upto date, making the exclusion list sensible and then working through it all. The hard/skilled part is the first bit. Trained monkeys like me can do the second bit.
Trained monkeys like me can do the second bit.
Automation FTW!
Ok. So if we can have clear and concise instructions to a trained monkey/gerbil level I am happy to be involved in getting the volunteer base behind this one. If it can be taught to me then any of our GSoC students who stay on would I am sure help out as well
If you get the Coding Standards and the .php_cs.dist in alignment - then no monkeys need to be involved. One simple command and the automation in php-cs-fixer fix
can resolve 99% of the issues.
The problems are:
libraries/vendor/joomla/cms-coding-standards/lib/Joomla-CMS/ruleset.xml
has a HUGE amount of exclusions.The quick automated fix for the time being would be
./libraries/vendor/bin/phpcs --standard=libraries/vendor/joomla/cms-coding-standards/lib/Joomla-CMS
php-cs-fixer fix
No monkeys needed.
There is no point having this conversation -no one has the balls to actually make the required merges...
Please keep your comments polite and respectful at all times. We still have GSoC students being asked to look at the github comments to learn so I respectfully ask that you keep comments polite and speak to others with the respect you would wish to be spoken to yourself. We have to set a standard for the students and we have explained to them, always talk about the issue and not the people. Once you start talking about the people it just becomes an argument and is not productive. Comments like that are not constructive or accurate and are not needed in a community project which relies on good will.
I would suggest your students use other open source projects to learn from, ones that take contributor safety seriously and that have a CoC process that actually works and doesn't just go to /dev/null at an OSM Board Level for the last 10 years - and proved again recently, repeatedly.
There is no point having "Students" work on Code Style & Code Quality, because THIS project's maintainers have publically stated they will flat out reject the changes because it makes their job hard.
It would be a total waste of the students time as it was mine - repeatedly - over many years.
The issue is that this project doesn't want to resolve its problems and move forward with optimising code quality, but equally wants to drag around 1990s FTP Layer... fact.
The currently published "Code Standards" are wildly out of date and do not take into account modern PHP development, standards or processes. They dont even promote short syntax arrays for goodness sake.
People, the maintainers, are the ones blocking PR's and progress, therefore it is right to talk about people. If they cannot be bothered to put in the work merging PRs then they should stand aside and let people that are willing to put in the work. Simple. Being a maintainer is more than just pressing a pretty merge button on a pre-prepared PR with no further involvement.
My comment was accurate, and correctly describes the current situation. There is no point having this conversation in this issue. Attacking me for simply stating the hard truth is also not right.
because THIS project's maintainers have publically stated they will flat out reject the changes because it makes their job hard.
Stop making false statements, I have said that this is not the right time to do such changes. It would bind resources we can use better at this time.
You were not attacked.
I simply asked you to keep all comments polite and respectful. As all people should in open forums.
I know for a fact that what your statement above is not an accurate reflection of the situation, but your opinion which you are fully entitled to. But if you make it public then others are also allowed to correct your opinion with their side, but also in a polite and respectful manner
because THIS project's maintainers have publically stated they will flat out reject the changes because it makes their job hard.
This is not a false statement. Do not accuse me of being a liar - you stated:
Changing a lot of files will make upstream merges more complicated as they are already. I don't will mege any of the code style PRs."
I know for a fact that what your statement above is not an accurate reflection of the situation
then others are also allowed to correct your opinion with their side
Really? prove it. Go on then. Im providing factual information - not opinion.
This is not a false statement. Do not accuse me of being a liar - you stated:
Changing a lot of files will make upstream merges more complicated as they are already. I don't will mege any of the code style PRs."
You should not cut off some of his words. You should quote the exactly what he said
Changing a lot of files will make upstream merges more complicated as they are already. I don't will mege any of the code style PRs. We can revisit this after we have released 4.0.0
That mean he is not doing now but later after 4.0.0 release
Doesn't look like "binding resources we can use better at this time." is the problem here...
You should not cut off some of his words.
I did not cut off any words. It was a direct quote.
Its the same excuses time after time... 5 years ago, now... it will never be done. When it is - you can tell me you told me so. But it will never get done.
Where do I accuse you of being a liar? I do not. I have been in the production meetings where very good and counter arguments are put. Often its simple to think that x is the right path to go, when with others perspective you learn there is an equally compelling counter argument. It's often not simple. In production I have been advocating for more communication and taken your points to the department.
This should be possible to debate in a factual and objective way but you take everything to the personal level.
So in light of your comments I will not take the subject any further.
I did not cut off any words. It was a direct quote.
That's not right. See his full comment here #33357 (comment)
Its the same excuses time after time... 5 years ago, now... it will never be done. When it is - you can tell me you told me so. But it will never get done.
I don't know about the past. But when we the right time comes and you make PR, I will be the one to test it, promise !
But when we the right time comes and you make PR, I will be the one to test it, promise !
Only for it to be rejected again...
Only for it to be rejected again...
That should not be right. I agree that we need to should the changes, but the right time not come yet. Again, came down :).
(@joomdonation Tuan I think the word you were looking for was "calm")
You know what the FULL IRONY is though... makes me laugh and nulls most of your arguments...
We have a maintainer that states he flat out refuses to merge the Code Style review PR's...
Changing a lot of files will make upstream merges more complicated as they are already. I don't will mege any of the code style PRs."
Then we have @wilsonge the official "Joomla Release Lead for Joomla 4" himself, MERGING Code Style Reviews of /plugins (over 75 files) 19 hours ago without a single peer review or comment apart from "Thanks"...
We can fix all the issues and it would be something we have to go thru one time, but with the option we don't have to do this again. I have been in this discussion 10 times and we never made the change. A good moment could be before 4.0.0, I will put it on the agenda of the next meeting.
Did it make it on to the agenda @rdeutz
Status | New | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-08-07 08:16:55 |
Closed_By | ⇒ | brianteeman | |
Labels |
Added:
?
Removed: ? |
Closed due to complete lack of interest
The standards are wildly out of date too, and dont take into account that Joomla 4 is now PHP 7+ and modern techniques, and the there is no statements on use the use of new PHP 8 features, pollyfills for those features, or policies on things like the use of named arguments or any policies on the new PHP 8 union types (just as examples)
But for PHP 7 even, There are no notes on the use of things like use of Spaceship Operator, Constant Arrays Using define, Null Coalescing Operator or Null Coalescing Operator just for example.
Also For example: phpStorm daily moans that class properties are not camelCase but snake_case because something in the repo tells phpStorm that Joomla wants camelCase yet most properties are snake_case...