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.

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)

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.

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

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?

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

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

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

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/

?

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

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 ?

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

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

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.

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

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)

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.

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

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

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

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.

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

Remove defined('*') or die where it's forbidden and doesn't make sense

Imho this needs the same changes as the project did for the libraries in the htaccess (nginx config, IIS, etc) to ensure that only the entry points are accessible from a request (root/index.php, root/api/index.php, root/administrator/index.php, root/administrator/components/com_joomlaupdate/extract.php, root/images, root/media are the only EXPOSED parts). Also probably there's a need to deal with templates as the php files shouldn't be accessible but for the legacy ones the static assets should be (this is getting too messy too soon...). My point is that there's need to be some counter acts to ensure at the very least that user uploaded files could not become an entry point (if they can then that's game over, security wise, kinda the current status quo)

Remove defined('*') or die where it's forbidden and doesn't make sense

Imho this needs the same changes as the project did for the libraries in the htaccess (nginx config, IIS, etc) to ensure that only the entry points are accessible from a request (root/index.php, root/api/index.php, root/administrator/index.php, root/administrator/components/com_joomlaupdate/extract.php, root/images, root/media are the only EXPOSED parts). Also probably there's a need to deal with templates as the php files shouldn't be accessible but for the legacy ones the static assets should be (this is getting too messy too soon...). My point is that there's need to be some counter acts to ensure at the very least that user uploaded files could not become an entry point (if they can then that's game over, security wise, kinda the current status quo)

true we should extend the .htaccess file but have to be careful, also a post install message should be added. Both are out of scope of this PR and should be done in it's own PR.

It's also on the idea map of joomla 5 so it doesn't hurt to bring this to 4.2 as optional security improvement. The 3rd party extensions that have own entry points should be mentioned in the post install message and the .htaccess file.

As someone who does that for a living: do not assume that it’s even possible to have a .htaccess or equivalent file or that the user has any say on their server configuration.

While most people are hosted on servers which allow .htaccess or web.config files and most of them can be used to block access to view template files it’s not a given. Delegating this responsibility to the user if they are using NginX is a recipe for disaster (the amount of times we had to help people who broke their NginX configuration is staggering). And yea there are servers out there where rewrites trait up don’t work.

Removing a simple, safe and sane protection to replace it with something the user has to remember doing, if they can, if they know how to is wither insane or inane. Especially when we’re talking about view template files which by their nature cannot be wrapped in a class and are de facto entry points!

Clearly, whoever thought of that had never built sites for real world clients, has never talked to real world users and has never done support on real world extensions.

Yeah, sure, defined or die doesn’t make much sense in the vast majority of classes but it definitely does in all view templates. Assuming that all .PHP files are the same is ignorant, dangerous and outright stupid.

Look, I am complaining here EVEN THOUGH this exact stupid decision will provide a massive boost to my security product sales. I can point out that the removal of that code is dangerous and my product fixes it. I am complaining because I don’t want basic security to be behind a paywall, even if I’m the one benefiting!

jexec or die

1. I get it that some files dont actually need this but having it in all files makes it so much easier to avoid accidental fp exposures. I was the person who went through 70% of the JED at the time and tested direct access to every php file. A huge percentage of files exposed the fp. Having jexec in every php file is much more secure than relying on a file we have no control over.
2. You can never rely on just adding a rule to the root htaccess or web.config. By its nature it will not be done by default on an upgrade and we know that many users will not understand what to do or why it is important. Even with post install messages any site that is updated by an external/remote service either manually or automatically (as seen with some hosts) will never see a message to update the htaccess or at least not until they next log in.
3. lightspeed servers do not 100% support htaccess. as a result some users will choose not to change a working setup or may not even be using an htaccesss file
4. I still regularly see live sites that are NOT using any htaccess (as demonstrated by the presence of index.php in the url)

Guys I know this, and if you look at the code also the PHP-FIG knows this in PSR-1-2-12 that's the reason why defined die is only removed in pure class files. Not more not less.

I also know that .htaccess is not the solution it's only hardening not more not less.

Guys I know this, and if you look at the code also the PHP-FIG knows this in PSR-1-2-12 that's the reason why defined die is only removed in pure class files. Not more not less.

We both acknowledged that in our comments.

However in the real world extension developers look at the core and follow its examples. To the non-experienced developer (most of us) it would not be clear that the file they have copied and are editing is no longer pure classes and now needs to have the jexec. Far far safer to have it for everything.

PS the jedchecker will need to be updated to take into account the changes here as well.

However in the real world extension developers look at the core and follow its examples. To the non-experienced developer (most of us) it would not be clear that the file they have copied and are editing is no longer pure classes and now needs to have the jexec. Far far safer to have it for everything.

Ok if extension developer copy our code, but way wouldn't they follow our codestyle then too? I mean if I copy code I have to know what I'm doing and actually if they really only copy-paste and don't add any stupid things (executing code in a definition file) to it then they are safe too. Also a better documentation make sense but the effort on the developer documentation seem to be stalled...

PS the jedchecker will need to be updated to take into account the changes here as well.

Maybe it's time to add phpcs to the jedchecker (if it's missing) with only a required subset (like this sniff PSR1.Files.SideEffects.FoundWithSymbols) would make sense.

@HLeithner I will address two things separately.

Regarding defined() or die.

I am only complaining about view template files. When you asked me to look into your PR the defined('_JEXEC') or die; was removed from all of them. This is BAD. These files, by their nature, will output something if accessed directly. The defined or die is a simple way to prevent that and works on all sites, always, without the user having to do anything.

Further to that, some classes and other non-view-template .php files might also need a defined or die statement. For example, look at the buffer class. Yes, there's a class in that file but there's also an executable statement which registers the buffer. If at some point the class is modified to use something else in Joomla which isn't present and cannot be loaded when accessing that .php file directly you might get an information disclosure vulnerability.

By removing defined or die you make it necessary for the user to do something. This is unrealistic for a mass distributed CMS which is self-managed by end users without technical knowledge. The part in italics is why I still need to write Admin Tools for Joomla and still make a living out of it. It's not that the .htaccess code it generates is a massive secret (I have contributed most of it to Joomla which has it in the Security section of the documentation), it's that people don't actually understand how to do it. It's an even worse situation for NginX and IIS' web.config.

Therefore I would say that at the very least view templates and possibly non-class files (including service providers!) MUST have the defined or die statement. If you cannot enforce this partially based on directory name then it's best to enforce it for the entirety of the CMS.

Regarding code style in JED checker.

Your proposal is nonsensical.

First of all, this sniff would fail on all service providers which are necessary for Joomla 4 native extensions. That's a big fail.

Second, there ARE legitimate use cases to BOTH define a class AND execute a code — and you're already doing that in the core. Custom autoloaders, buffer classes, CLIApplication or WebApplication instances and I could keep going on.

Putting a sniff like that in the jedchecker which is operated by people who do not understand these fine points would require us developer to write kludgy, unsafe and potentially insecure code to appease a useless check! The worse part is that the people who will be called to make a decision based on the results of the jedchecker are not developers, they definitely cannot do code audits and would end up rejecting perfectly safe and working software because it failed a pointless check while accepting outright insecure software because it passes the automated checks. How does that even make sense?!

Conclusion

The core can use whichever code style it wants. If you want PSR-12, sure, whatever, who cares.

The core MUST NOT remove defined or die checks from any .php files which are not pure class files. This is the quickest way to security issues.

You must not enforce pointless code checks on JED listings UNLESS you have the budget to hire very experienced developers to do manual code audits and write up detailed reports on why the code is rejected, with an appeal process.

Ok if extension developer copy our code, but why wouldn't they follow our codestyle then too? I mean if I copy code I have to know what I'm doing and actually if they really only copy-paste and don't add any stupid things (executing code in a definition file) to it then they are safe too

Thats the point of why someone copies the code. they don't know what they are doing. they just hack around until it works for them including doing stupid things. You should be able to see this in action in many of my pr

@nikosdion I see your concerns and try to solve them

Regarding defined() or die.

the defined('_JEXEC') or die; was removed from all of them

That's not true they are only removed if PHPCS thru a PSR1.Files.SideEffects.FoundWithSymbols error. Then the defined die will be clean. There where only a couple of files that has FoundWithSymbols after the cleaning, I checked all of them and added a * @phpcs:disable PSR1.Files.SideEffects to the header comment in the prepare pr. In this PR you see all exception or cleanups I have done to make the rest without manual changes.

For example the BufferStreamHandler still have a defined die: https://github.com/HLeithner/joomla-cms/blob/4.2-psr12/libraries/src/Utility/BufferStreamHandler.php#L10-L15

Not sure if you mean with "view template files" the PHP/HTML files in the tmpl folder or the files holding the HtmlView classes. in the first case, I didn't touched any layout/tmpl file yet. for the HtmlView I'm not sure why to handle this different then the rest of the classes? We never have executable code in this classes and wonder why a 3rd party developer should have (but even then s/he has to add the defined die to it).

Regarding the service providers, The defined die is still there because there is code executed, please have a look at com_config (I picked a random component)

About the htaccess stuff, I know that this is only an additional protection and the CMS (and of course all additional extensions) must be safe without it. I never used admin tools so I can't say what you are doing with the htaccess file but I would expect you add some hardening too right?

Regarding code style in JED checker.

Your proposal is nonsensical.

is not true look above, we have many service providers and all include the defined die function call.

Second, there ARE legitimate use cases to BOTH define a class AND execute a code — and you're already doing that in the core. Custom autoloaders, buffer classes, CLIApplication or WebApplication instances and I could keep going on.

That's true and in all of them we still have the defined die (except if they define the _JEXEC constant).

In this file you see all exceptions which are per definition impossible to have a defined die or are still have a defined die.

About the JED checker first it was a suggestion and doesn't mean that it's the final idea, for example we could write/extend the sniff for SideEffects to check if a defined die exists in a mixed file (or a file that doesn't define any symbol).
Sadly I really know too less about the jed checker, how it works, why it works and if it does anything. What I think is extension developers need help to create secure extensions. And copy paste blindly is never a good idea. That's the reason I support the developer documentation initiative. @weeblr is in charge to build this up he already started with the template and basic system.

The core MUST NOT remove defined or die checks from any .php files which are not pure class files. This is the quickest way to security issues.

As already mention that's the case.

You must not enforce pointless code checks on JED listings UNLESS you have the budget to hire very experienced developers to do manual code audits and write up detailed reports on why the code is rejected, with an appeal process.

A developer who knows what he is doing can and should ad the phpcs:disable PSR1.Files.SideEffects.FoundWithSymbols to the header of the file. And no I don't want to add useless checks to the JED checker.

I am looking at the diff in GitHub. It is possible that because of the size of the PR it had not calculated the diff correctly or something? Now I cannot find any view templates without the defined or die.

Sorry! It's not the first time GitHub fails me on large PRs but I didn't have access to my computer either (I was on an iPad) :(

About the JED checker first it was a suggestion and doesn't mean that it's the final idea, for example we could write/extend the sniff for SideEffects to check if a defined die exists in a mixed file (or a file that doesn't define any symbol).

Thank you for this clarification! Yes, this makes sense. The way it was phrased before I had understood the opposite, that you would either not allowed mixed use files or that you'd enforce non-mixed file to not have a defined or die, both of which didn't make sense. If you are only intending to enforce mixed files to have the defined or die, please, yes, that's what the jedchecked is (supposed to be) doing already.

And copy paste blindly is never a good idea. That's the reason I support the developer documentation initiative. @weeblr is in charge to build this up he already started with the template and basic system.

Amen!

@nikosdion can you get the script to work as it doesnt for me

@brianteeman The script seems to no longer be there, there's a different script psr12_converter.php. I am wondering what should be the new sequence of commands, though. My best guess is:

php build/psr12/clean_errors.php 
php build/psr12/psr12_converter.php --task=cbf `pwd`
php build/psr12/psr12_converter.php --task=cs `pwd`

However the last step seems to fail(?):

Joomla! PSR-12 Converter
=======================

Tasks will be executed: CS
Files and Folders:
/Users/nicholas/Projects/master/joomla4

Check /Users/nicholas/Projects/master/joomla4
<?xml version="1.0" encoding="UTF-8"?>
<phpcs version="3.6.2">
</phpcs>
Error PHPCS completed with error code: 1 

I do get a file build/tmp/psr12/result.html which reports an error which, in retrospect, is bogus and needs an exception:

Check
/Users/nicholas/Projects/master/joomla4/configuration.php

Errors: 1 Warnings: 0 Fixable: 0

Line: 3 Column: 1 Fixable: No Severity: 5 Rule: PSR1.Classes.ClassDeclaration.MissingNamespace
Each class must be in a namespace of at least one level (a top-level vendor name)

Well, the configuration.php must never be in a namespace.

My other problem with doing those things is that I am only testing against the already converted repository which doesn't make much sense. Wouldn't it make more sense to have the conversion script and all of its associated files in its own PR? Like, what am I even testing running this script?

@nikosdion

The script seems to no longer be there, there's a different script psr12_converter.php. I am wondering what should be the new sequence of commands, though. My best guess is:

I updated the script and moved it to the preparation pr #37681

You only nee to execute

php build/psr12/psr12_converter.php --task=cms

This does the compelte process including git commits.

you can do the steps separated

This will only generate the report in build/tmp/psr12/result.html and the build/tmp/psr12/cleanup.json

php build/psr12/psr12_converter.php --task=cs

Running the clean_errors.php will load the cleanup.json from the cs task (but before you run this you should run the tasks cs,cbf to get a preprocessed psr12 version)

php build/psr12/clean_errors.php

After this you need to run the cbf task again because the cleanup script doesn't do clean line breaks and endings

php build/psr12/psr12_converter.php --task=cbf

Check
/Users/nicholas/Projects/master/joomla4/configuration.php

Errors: 1 Warnings: 0 Fixable: 0

Line: 3 Column: 1 Fixable: No Severity: 5 Rule: PSR1.Classes.ClassDeclaration.MissingNamespace
Each class must be in a namespace of at least one level (a top-level vendor name)

Well, the configuration.php must never be in a namespace.

hmm didn't run the script in a directory with configuration.php, will add it to the exception list.

My other problem with doing those things is that I am only testing against the already converted repository which doesn't make much sense. Wouldn't it make more sense to have the conversion script and all of its associated files in its own PR? Like, what am I even testing running this script?

As already mentioned I moved it already to the preparation pr #37681

if you check out this pr and run --task=cms you get this pr before the manual tasks listed at the end of the psr12_converter script.

Hope that helps.

PS: @brianteeman and I found out that the exclude-patterns of the ruleset.xml doesn't work for him... not sure if he found already a solution.

yes i found the error and commented on the other pr. from what I read it shouldnt work at all without the changes I commented

!!!!!!!!!! DANGER! DO NOT REMOVE THIS LINE !!!!!!!!!!

When you run the update the Model creates the file restoration.php which is loaded by extract.php. The restoration.php file contains executable code, therefore requires a defined or die statement. However, extract.php runs outside of Joomla itself for the reasons I explained both in its PR and the comments on the extract.php file itself. Essentially, the only way to guard against a timeout during the extraction is to run the extraction on its own self-contained file (since Joomla will be in a fluid, possibly very broken, state during the extraction).

If you remove this line the restoration.php file will die() immediately when included and the update will not take place.

If you remove the defined or die check from the generated restoration.php you would put Joomla users at risk.

Please add me as a code owner for the extract.php file. Until you resolve the issue with non-members of the repository not being notified even when they are code owners please at-mention me on any change affecting extract.php. I reckon I can catch issues with proposed changes to this file faster than anyone.

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

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

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

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

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!

. 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

. 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

Does this mean that this is now postponed until 4.3?

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