Imo the PHP 5.6 test should be abandoned since the support of PHP5.6 was stopped since end of 2018.

Imo the PHP 5.6 test should be abandoned since the support of PHP5.6 was stopped since end of 2018.

How about reviewing the task that was executed on PHP 5.6 instead of just flat out saying it should be abandoned? Or, you can just disable PHPCS for this repo then you don't have to worry about CI failing because your code style is not compliant with the Joomla coding standards.

@datsepp increased the required PHP version to 7.0.x as you can see in composer.json, so the failing test is expected. He just relayed on the PHP recommendation - but if you prefer, we will remove this change and accept PHP5.6. The question is: do we really need to support PHP 5.6 when Joomla 4 already requires PHP 7.2.x?

don't forget that j.3 will be supported till....... and php requirements for j.3 is 5.3.10,
as it seems that there will be only 1 com_patchtester version which should work for both 3.x and 4.x...

Did anyone actually look at the CI configuration or did you all stop reading at 5.6 and just arbitrarily decide "oh, hey, this PHP version isn't supported anymore, that's a problem"?

Since it seems my question is rhetorical and I already know the answer to this, let me explain how it works.

This repository enforces the Joomla Coding Standards, presently the 1.x version of them, which relies on PHP_CodeSniffer version 1.x. To my knowledge, this version of PHP_CodeSniffer is not supported on any PHP 7 build. So, logically, the coding standards are checked on the newest version of PHP that the version of the tool in use supports, PHP 5.6.

That wasn't hard, was it?

The question is: do we really need to support PHP 5.6 when Joomla 4 already requires PHP 7.2.x?

Well, since you have decided to rewrite this component in a way that can only support 4.0, then yes, you are free to drop support for older PHP versions, and Joomla 3.x as a whole. Actually, you can probably drop GitHub support as a whole too since it seems you aim to only support builds for the CMS repo coming from Drone, ignoring the fact that this component could actually be used with any GitHub repository and any Joomla extension that follows the CMS file structure. You can probably drop Crowdin support as well as it seems you're now manually updating translation files and not translating all messages that reach the user interface.

o my knowledge, this version of PHP_CodeSniffer is not supported on any PHP 7 build. So, logically, the coding standards are checked on the newest version of PHP that the version of the tool in use supports, PHP 5.6.

@mbabker seems it requires 5.4+
https://github.com/squizlabs/PHP_CodeSniffer/wiki/Requirements

That is the current version of the tool, not the version in use right now.

Until Joomla 4, there were no compilable files in Joomla. Therefore, our proposed changes are not required for J3.x and older! You can still use the patchtester as-is for testing these versions. After discussing the CI solution for the patchtester with some guys of the Joomla Community at the Joomla Day Germany this weekend, we came to the conclusion that there is no need to integrate this into the patchtester for J3.x and older and therefore we decided to look forward to J4 and create this solution for J4 only.

Actually, you can probably drop GitHub support as a whole too since it seems you aim to only support builds for the CMS repo coming from Drone, ignoring the fact that this component could actually be used with any GitHub repository and any Joomla extension that follows the CMS file structure.

We keep this feature, you can switch between CI and Github at any given time. We do not want to drop functionality, but keep it. And you can't test J4 with the patchtester as-is because of the compilable assets used.

Please be mindful of your words. The claim “you can’t test J4 with the patchtester as-is” is 100% wrong. You CAN test a vast majority of patches for J4, but not ALL patches. This may seem like a minor difference, but that change in verbiage makes a huge difference in communication and the responses it will get from others (what you are saying is that the component does not work at all and that could not be further from the truth).

I also don’t get why all of a sudden you feel like you need two versions of patchtester for two versions of Joomla. All past maintainers of this component were able to provide one build compatible with all Joomla versions (most of you won’t remember a time when 2.5 was also supported), requiring separate versions of the component for separate versions of Joomla creates end user confusion. There has never been “patchtester for J3.x” and “patchtester for J4”, there should never be a need to do so either and the fact you are implying there is means that something is broken in your approach or you have made the component too complex for the same end user that this component is targeted at.

It is possible now to test the patchtester with builds from the ci server. If you don't want to use the ci integration it is also possible to turn it off in the ci settings - f.e. if you plan to continue using j3 or want to test patches on j4 without compiled js/css files. Feel free to comment or ask if you have any further questions :)

@mbabker I have some questions regarding crowdin and I've written you a direct message on crowdin as well. If you have time I would be glad to hear from you. Thank you for your feedback so far~

@datsepp
Thanks for investigation & your help.
I found it under the options :-)

It is possible now to test the patchtester with builds from the ci server. If you don't want to use the ci integration it is also possible to turn it off in the ci settings ...

Do JS code changes work with whatever browsers J3 supports?

Do JS code changes work with whatever browsers J3 supports?

They do mostly. IE 8 supports the proprietary .attachEvent method instead,
but there also exists a workaround if necessary so.

Thank you for reviewing~.

composer.lock needs to be regenerated.

They do mostly. IE 8 supports the proprietary .attachEvent method instead,
but there also exists a workaround if necessary so.

What about the use of let? Seems it's unsupported https://caniuse.com/#search=let

What about the use of let? Seems it's unsupported https://caniuse.com/#search=let

Yeah, I saw that yesterday at night as well, had not the time to change it.
Thanks for pointing that out, I replaced this as well as the arrow functions.

...and thanks again for reviewing~.

in the hope to try to have something working without hard debate, or with the goal to have something working for the Worldwide PBF event, despite different opinion.... my silly proposal is to a have an hard fork of com_patchtester....so with that in mind .... i've made a silly fork of com_patchtester + this draft pr that in my humble opinion could be a way to superate discussion and try to do something quite fast to be delivered before PBF......or at least i'm trying to do something i'm convinced could be a good compromise

https://github.com/alikon/com_pbf

the repo is open to everyone....i swear i'll merge all pr's that will be submitted

Some things I ran into when trying to test this:

Go to https://ci.joomla.org/artifacts/joomla-cms/4.0-dev//patchtester/build.zip and download the zip file with your browser after you inserted the pull request id of you pull request in the corresponding place of the url.

The URL seems to be https://ci.joomla.org:444/artifacts/joomla/joomla-cms/4.0-dev/<pull-request-id>/patchtester/build.zip. Can you please correct this in the testing instructions?

I tried to apply joomla/joomla-cms#26210 and joomla/joomla-cms#26360, but both failed when trying to move files like "administrator/components/com_media/package-lock.json". To me, the ZIP files seem to contain many files not directly related to the patch, like the package-lock.json which is write-protected and thus can't be moved or changed. And the amount of files contained in a ZIP seems to grow the older a PR gets (correct me if I'm wrong). For example, for joomla/joomla-cms#26360, I'm getting a 12MB download.

Could this be reduced somehow? Maybe by letting drone attempt to rebase before creating the ZIP, so the ZIP really only contains the changes? (I'm not an expert on CI or NPM, so feel free to correct me if I'm mistaken)

The URL seems to be https://ci.joomla.org:444/artifacts/joomla/joomla-cms/4.0-dev/<pull-request-id>/patchtester/build.zip. Can you please correct this in the testing instructions?

Will do! Thanks for pointing that out, I missed that one with the last changes. EDIT: Done.

I tried to apply joomla/joomla-cms#26210 and joomla/joomla-cms#26360, but both failed when trying to move files like "administrator/components/com_media/package-lock.json". To me, the ZIP files seem to contain many files not directly related to the patch, like the package-lock.json which is write-protected and thus can't be moved or changed.

You are correct with this one, there are still a lot of files which need to be excluded from building the archives, which I will fix soon.

And the amount of files contained in a ZIP seems to grow the older a PR gets (correct me if I'm wrong). For example, for joomla/joomla-cms#26360, I'm getting a 12MB download.

Could this be reduced somehow? Maybe by letting drone attempt to rebase before creating the ZIP, so the ZIP really only contains the changes? (I'm not an expert on CI or NPM, so feel free to correct me if I'm mistaken)

This would be a possibility, but I need to check how easily the rebase can be automated (if it can not be done automatically)... Maybe the file size of the archive will be reduced with the changes in the compare step aswell.

Can you please update this branch to the latest changes? Then I would merge this.

Thank you!