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

Adapt package structure #165

Closed
wants to merge 29 commits into from
Closed

Adapt package structure #165

wants to merge 29 commits into from

Conversation

schlessera
Copy link
Member

This PR makes changes to the package structure to fit the one from the bundled plugins:

  • Use WP_CLI\Doctor as the namespace root
  • Use PSR-4 autoloader
  • Use latest wp-cli-tests framework with Behat v3

@danielbachhuber
Copy link
Member

@schlessera What do you think about punting on the namespace change, and simply applying the safe bits?

@swissspidy swissspidy requested a review from a team November 3, 2023 13:11
@swissspidy
Copy link
Member

@schlessera @danielbachhuber Apologies for the noise here, but I thought I'd pick this up and help fix all the failing tests. This is now ready for review!

I don't mind the namespace change, I think it should be fine. Just noting that this handbook page needs updating after merge: https://github.com/wp-cli/handbook/blob/7c792d2a11448dab77f23d8055f82553e6dff2a6/doctor-customize-config.md

@swissspidy swissspidy added this to the 2.2.0 milestone Nov 3, 2023
Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really a fan of this pull request as it currently is. It will cause a lot of breakage for existing users, for very little real-world benefit.

If we're committed to renaming the classes, then I think we should add backcompat for the existing classes (and make sure there's backcompat for the doctor.yml file).

doctor.yml Outdated Show resolved Hide resolved
@swissspidy
Copy link
Member

Makes sense.

Btw, any idea why this one test is failing on 6.2?
The 3.7 one I haven‘t looked into yet

@danielbachhuber
Copy link
Member

Btw, any idea why this one test is failing on 6.2?

$ wp core version
6.2
$ wp core check-update
+---------+-------------+-----------------------------------------------------------------------+
| version | update_type | package_url                                                           |
+---------+-------------+-----------------------------------------------------------------------+
| 6.2.3   | minor       | https://downloads.wordpress.org/release/wordpress-6.2.3-partial-0.zip |
| 6.3.2   | major       | https://downloads.wordpress.org/release/wordpress-6.3.2.zip           |
+---------+-------------+-----------------------------------------------------------------------+
$ wp doctor check core-update
+-------------+--------+----------------------------------------------------------------------+
| name        | status | message                                                              |
+-------------+--------+----------------------------------------------------------------------+
| core-update | error  | Updating to WordPress' newest minor version is strongly recommended. |
+-------------+--------+----------------------------------------------------------------------+
Error: 1 check reports 'error'.

It looks like main is passing because behat.yml isn't detected and no tests are being run.

@swissspidy
Copy link
Member

Yes, that I realized. That‘s why I did all these commits to fix tests. Just thought it was weird that it failed for that particular job. But I can adapt the test 👍

@swissspidy
Copy link
Member

OK looks like @require-wp-latest doesn't seem to actually work 🤔

@danielbachhuber
Copy link
Member

OK looks like @require-wp-latest doesn't seem to actually work 🤔

@swissspidy Lovely. Maybe it's not actually a thing!

For Scenario: WordPress is up to date, you could run wp core update prior to wp doctor check core-update. I'd probably do the same for Scenario: Use --spotlight to focus on warnings and errors.

@swissspidy
Copy link
Member

Thanks, yeah I‘ll try that again.

Amd then I guess I‘ll review all existing usage of @require-wp-latest in the code base 🤔

Any idea on the WP 3.7 test? Can‘t run them locally as I don‘t have old PHP installed.

@danielbachhuber
Copy link
Member

Any idea on the WP 3.7 test? Can‘t run them locally as I don‘t have old PHP installed.

Nothing immediately obvious, no.

@swissspidy
Copy link
Member

Now there are some random errors from extension command as well, like Warning: twentyeleven: version higher than expected.. Core bumped their versions yesterday
but they haven't been released yet in the theme directory. So probably should go away after the 6.4 release tomorrow 🤷

I'll leave this PR be for now, maybe someone else wants to pick it up.

@swissspidy
Copy link
Member

@ernilambar Since you worked on this repo recently, maybe this PR is something you'd be interested in bringing over the finish line?

@ernilambar
Copy link
Member

@ernilambar Since you worked on this repo recently, maybe this PR is something you'd be interested in bringing over the finish line?

Sure. I will work on it.

@ernilambar
Copy link
Member

Closing in favor of #194

@ernilambar ernilambar closed this Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants