search

joomla-extensions / jedchecker

Joomla extension to check components, modules or plugins for possible problems for submission to the JED -> Translations: https://joomla.crowdin.com/joomla-official-extensions
38 stars 28 forks source link

PHP syntax check #182

Open dryabov opened 2 years ago

dryabov commented 2 years ago

This PR adds new rule "Syntax check" that checks PHP syntax using nikic/php-parser package.

dryabov commented 2 years ago

BTW, what do you think about running composer install (probably followed by composer update) as a GitHub action (see https://github.com/php-actions/composer)? Currently it's necessary to either trust the committer, or verify all files in the vendor directory.

Llewellynvdm commented 2 years ago

Hi @JazParkyn not sure why @dryabov is still listed as a contributor, he did join the JED as a Member (check the minutes of January 28th, 2022, it could be because he is only going to work on the JED checker as his main focus and the volunteer portal did not have that as a Role option at the time of my end of term.

@dryabov I would encourage you to communicate the merit of this PR, and your membership in the JED directly with the JED team on Glip, mostly since I am no longer a member or even a contributor towards the JED team, as result my time here "helping you" is very limited, and I am not willing to make decisions about the JED checker being outside of the team conversation. Yet with you as a member, you are able to join the conversation on Glip and request access to mange this repository as a maintainer, or ask who will help you do so. As your code contribution has been amazing, and I would highly recommend you as the new JED checker maintainer, but you will need a partner who can help double check things with the team and basically avoid losing contact with the objectives and standards the JED team has.

Now back to this PR it seems to me that the new team leader @JazParkyn is unhappy with the size of he JED checker package at this point, with the added new composer packages, and I think she is right since an extension of this size will normally be limited in its usefulness in the larger Joomla world of shared hosting.

You will have to look at this feature as a nice to have, and you will have to think down the lines of installing it separately from the actually JED checker package. Further more there are some reluctance to adding composer packages to extensions, as I am sure you understand. So setting up an library extension will be the more correct approach, and then adding an easy to install button that will take the user to the normal Joomla installer and install these packages as a library will probably be more acceptable to everyone involved. Once the JED checker notices that these libraries are installed, then the feature will automatically take action, but with yet another clear option to turn it on or off, and or uninstall again.

These are my quick suggestions to help these initiatives move forward, as this new feature will be an amazing help to the JED team members who are validating new extensions, and I hope this will be the first step towards giving developers the option to see deprecated code in their extensions... as that was the main objective of going this direction as per request of the production department.

I did mention to @JazParkyn my willingness to continue helping with the JED checker, but have since realized it will not make sense if I am not a member, and at this point that is out of the question.

dryabov commented 2 years ago

I'm sorry, I've been very busy the last days. Here are my main thoughts:

As for the size of JEDChecker, I don't think it's such a big problem. It's used mainly by the JED team, extensions developers, and advanced users. Adding PHP-Parser for syntax checking will increase the zip archive size by 280Kb (the current size is 510Kb). And, to be honest, I'm not sure that anyone even uses JEDChecker on a shared hosting.

I'm currently working on adding PSALM code to JEDChecker to keep track of variable types and find usage of the legacy Joomla API. And this will significantly increase the archive size (by about 3Mb), and I really would like to put it in a separate package, not only because of the size requirements, but also the available RAM usage (it requires a minimum of 200Mb, and by default PHP has a limit of 128MB only).

Although, if this check would be a separate addon, then it might make sense to put the aforementioned syntax check into a separate addon too, because the only addon looks somehow strange.

mfleeson commented 2 years ago

I'd be happy to help. I've recently been working on a project using installable libraries for joomla.