?
avatar brianteeman
brianteeman
26 Apr 2021

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.

  • joomla/cms-coding-standards
  • joomla/coding-standards

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

avatar brianteeman brianteeman - open - 26 Apr 2021
avatar joomla-cms-bot joomla-cms-bot - change - 26 Apr 2021
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - labeled - 26 Apr 2021
avatar PhilETaylor
PhilETaylor - comment - 26 Apr 2021

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...

avatar PhilETaylor
PhilETaylor - comment - 26 Apr 2021

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.

avatar brianteeman
brianteeman - comment - 26 Apr 2021

That was my "guess" as well but the quoted documentation says that joomla/coding-standards is for everything

avatar PhilETaylor
PhilETaylor - comment - 26 Apr 2021

Lots of unfinished commentary in joomla/coding-standards#143

avatar Bakual
Bakual - comment - 26 Apr 2021

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.

avatar brianteeman
brianteeman - comment - 26 Apr 2021

Check the exclude list - a lot bigger than that

avatar PhilETaylor
PhilETaylor - comment - 26 Apr 2021

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

avatar rdeutz
rdeutz - comment - 26 Apr 2021

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.

avatar PhilETaylor
PhilETaylor - comment - 26 Apr 2021

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

avatar softforge
softforge - comment - 26 Apr 2021

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?

avatar PhilETaylor
PhilETaylor - comment - 26 Apr 2021

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

avatar rdeutz
rdeutz - comment - 26 Apr 2021

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.

avatar brianteeman
brianteeman - comment - 26 Apr 2021

That's exactly why I was looking at this today.

avatar PhilETaylor
PhilETaylor - comment - 26 Apr 2021

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.

avatar brianteeman
brianteeman - comment - 26 Apr 2021

@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.

avatar PhilETaylor
PhilETaylor - comment - 26 Apr 2021

Trained monkeys like me can do the second bit.

Automation FTW!

avatar softforge
softforge - comment - 26 Apr 2021

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

avatar PhilETaylor
PhilETaylor - comment - 26 Apr 2021

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:

  • The Joomla Coding Standards are disputed, non-modern, and need to be improved.
  • The PhpCsFixer Configuration is only PSR1 PSR2 and a few Symfony rules (that's what started this conversation)
  • The PhpCsFixer Configuration is only covering /libraries and /libraries/src/* folders
  • The 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

  • update .php_cs.dist to cover the whole of Joomla (excluding libraries/vendor and 3pd stuff)
  • ensure the rules in .php_cs.dist and the ruleset.xml files in cms-coding-standards and coding-standard libs MATCH in their logic (These are what Drone uses when it runs ./libraries/vendor/bin/phpcs --standard=libraries/vendor/joomla/cms-coding-standards/lib/Joomla-CMS
  • run php-cs-fixer fix
  • marvel at the automated fixes of 1000s of files...

No monkeys needed.

avatar PhilETaylor
PhilETaylor - comment - 27 Apr 2021

There is no point having this conversation -no one has the balls to actually make the required merges...

avatar softforge
softforge - comment - 27 Apr 2021

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.

avatar PhilETaylor
PhilETaylor - comment - 27 Apr 2021

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.

avatar rdeutz
rdeutz - comment - 27 Apr 2021

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.

avatar softforge
softforge - comment - 27 Apr 2021

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

avatar PhilETaylor
PhilETaylor - comment - 27 Apr 2021

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."

avatar PhilETaylor
PhilETaylor - comment - 27 Apr 2021

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.

avatar joomdonation
joomdonation - comment - 27 Apr 2021

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

avatar PhilETaylor
PhilETaylor - comment - 27 Apr 2021

Doesn't look like "binding resources we can use better at this time." is the problem here...

@softforge

Screenshot 2021-04-27 at 16 56 49

@rdeutz

Screenshot 2021-04-27 at 16 57 04

avatar PhilETaylor
PhilETaylor - comment - 27 Apr 2021

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.

avatar softforge
softforge - comment - 27 Apr 2021

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.

avatar joomdonation
joomdonation - comment - 27 Apr 2021

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 !

avatar PhilETaylor
PhilETaylor - comment - 27 Apr 2021

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...

avatar joomdonation
joomdonation - comment - 27 Apr 2021

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 :).

avatar brianteeman
brianteeman - comment - 27 Apr 2021

(@joomdonation Tuan I think the word you were looking for was "calm")

avatar PhilETaylor
PhilETaylor - comment - 27 Apr 2021

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"...

avatar brianteeman
brianteeman - comment - 13 Jun 2021

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

avatar brianteeman brianteeman - change - 7 Aug 2021
Status New Closed
Closed_Date 0000-00-00 00:00:00 2021-08-07 08:16:55
Closed_By brianteeman
Labels Added: ?
Removed: ?
avatar brianteeman brianteeman - close - 7 Aug 2021
avatar brianteeman
brianteeman - comment - 7 Aug 2021

Closed due to complete lack of interest

Add a Comment

Login with GitHub to post a comment