-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move to YoastCS 2.0 and bring in line with other plugins autoloading #64
Conversation
@jrfnl @moorscode for some reason in the build the files still have the old names (PSR-4 style), even though in the branch they're renamed... Github fluke? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jdevalk I've done a quick review of the principle of the PR and the ruleset. I've only done a cursory look-over of the fixes in the plugin code files. (In Canada at the moment. I can look in more detail next week if needs be)
Any comment I've left for the code fixes for one particular line should be seen as a generic comment to be applied in all relevant places in the whole PR.
Aside from the inline feedback:
Please add the <arg name="basepath" value="./"/>
directive. That is used by some of the YoastCS native sniffs to know the plugin root.
Please add the below snippet to indicate that the code is in the src
directory. This is used for the path to namespace translation & check.
<rule ref="Yoast.NamingConventions.NamespaceName">
<properties>
<!-- Treat the "src" directory as the project root for path to namespace translations. -->
<property name="src_directory" type="array">
<element value="src"/>
</property>
</properties>
</rule>
for some reason in the build the files still have the old names (PSR-4 style), even though in the branch they're renamed... Github fluke?
The Notification.php
file as well as the Taxonomies.php
file should be lowercase, they are now uppercase. AFAICS, that's where it's going wrong.
And once the above two extra snippets have been added, you will see seven more issues.
These are caused by the WordPress_Plugins
directory not complying with the expected directory naming of lowercase, dash-separated, i.e. the directory should be called wordpress-plugins
.
Once the directory name has been corrected, those seven will disappear.
As a side-note: the ruleset could do with minor syncing with the other plugin rulesets as for the inline documentation, but if you like I'll fix that up next week. Not a stopper for this PR.
Travis: as after this PR, the plugin will be clean, please remove the -q --runtime-set ignore_warnings_on_exit 1
from the command in the Travis script.
Some other things I noticed while looking at the PR/wider context of the plugin:
- A quick look at the Travis script shows me there are more tweaks needed, looks like PHP 5.3 is still being used there while it is not supported and the script also still contains references to PHP 5.2 and PHP 7.2 should probably be replaced with PHP 7.4.
- The
composer.json
file does not annotate the minimum supported PHP version. - The
.gitignore
file is missing ignores for the typical PHPCS overload files.
If you like I can look those kind of things over in more detail and send in a PR to fix them when I'm back next week.
Thx for all your input on this @jrfnl. Going to merge this, I think the only thing left is:
If you could do those in a separate PR later that'd be appreciated! |
@jdevalk 👍 I'll add this plugin to my regular scan round now and will send in additional PRs where needed. |
Summary
This PR can be summarized in the following changelog entry:
composer check-cs
andcomposer fix-cs
scripts.Relevant technical choices:
Yoast\Test_Helper
toYoast\WP\Test_Helper
to be in line with other plugins.Milestone
Test instructions
This PR can be tested by following these steps:
Fixes #62
Fixes #63