Skip to content
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

Merged
merged 10 commits into from
Apr 3, 2020

Conversation

jdevalk
Copy link
Contributor

@jdevalk jdevalk commented Feb 29, 2020

Summary

This PR can be summarized in the following changelog entry:

  • Moves the entire plugin to YoastCS 2.0.
  • Implemented composer check-cs and composer fix-cs scripts.
  • Fixes a notice on the admin page when a plugin wasn't installed.
  • Flushes the rewrite rules after enabling the books and movies post type.

Relevant technical choices:

  • Moved away from PSR_4 autoloading to classmap, so we could be in line with other plugins.
  • Changed the namespace from Yoast\Test_Helper to Yoast\WP\Test_Helper to be in line with other plugins.

Milestone

  • I've attached the next release's milestone to this pull request.

Test instructions

This PR can be tested by following these steps:

Fixes #62
Fixes #63

@jdevalk jdevalk added this to the 1.5 milestone Feb 29, 2020
@jdevalk
Copy link
Contributor Author

jdevalk commented Feb 29, 2020

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

Copy link
Contributor

@jrfnl jrfnl left a 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.

composer.json Outdated Show resolved Hide resolved
composer.json Show resolved Hide resolved
phpcs.xml.dist Outdated Show resolved Hide resolved
phpcs.xml.dist Outdated Show resolved Hide resolved
phpcs.xml.dist Show resolved Hide resolved
src/admin-notifications.php Outdated Show resolved Hide resolved
src/inline-script.php Show resolved Hide resolved
src/post-types.php Outdated Show resolved Hide resolved
src/schema.php Outdated Show resolved Hide resolved
src/schema.php Outdated Show resolved Hide resolved
@jdevalk
Copy link
Contributor Author

jdevalk commented Apr 3, 2020

Thx for all your input on this @jrfnl. Going to merge this, I think the only thing left is:

  • The .gitignore file is missing ignores for the typical PHPCS overload files.
  • 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.

If you could do those in a separate PR later that'd be appreciated!

@jdevalk jdevalk merged commit ec84b45 into develop Apr 3, 2020
@jdevalk jdevalk deleted the jdv/yoast-cs-2.0 branch April 3, 2020 08:20
@jrfnl
Copy link
Contributor

jrfnl commented Apr 3, 2020

@jdevalk 👍 I'll add this plugin to my regular scan round now and will send in additional PRs where needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants