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

upkeep/163: Fix fatal error on PHP 5.6 #166

Merged
merged 10 commits into from
Oct 10, 2023
Merged

upkeep/163: Fix fatal error on PHP 5.6 #166

merged 10 commits into from
Oct 10, 2023

Conversation

Sidsector9
Copy link
Member

Description of the Change

This PR does the following:

  1. Moves the class Simple_Page_Ordering to a separate file.
  2. Require class-simple-page-ordering.php only when platform validation passes.

Closes #163

How to test the Change

Please follow steps from the issue.

Changelog Entry

Fixed - Fatal errors on PHP 5.6

Credits

Props @peterwilsoncc @Sidsector9

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@Sidsector9 Sidsector9 requested a review from a team as a code owner October 5, 2023 12:01
@Sidsector9 Sidsector9 requested review from faisal-alvi and removed request for a team October 5, 2023 12:01
@github-actions github-actions bot added this to the 2.6.0 milestone Oct 5, 2023
@github-actions github-actions bot added the needs:code-review This requires code review. label Oct 5, 2023
Copy link
Member

@iamdharmesh iamdharmesh left a comment

Choose a reason for hiding this comment

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

@Sidsector9 Thank you for the PR.

PR adds composer dependency to the plugin but it seems we have ignored the vendor folder in .distignore file. It may result in vendor folder missing in the release zip. Could you please help to check it once?

Also, we have to add steps for composer package installation in our GH action workflows. especially in deploy workflow. Could you please help with it?

Thank you.

@Sidsector9 Sidsector9 requested a review from jeffpaul as a code owner October 5, 2023 15:19
@jeffpaul jeffpaul mentioned this pull request Oct 5, 2023
15 tasks
@Sidsector9 Sidsector9 force-pushed the upkeep/163 branch 2 times, most recently from eafce4f to f5d13d4 Compare October 5, 2023 17:44
<?php
}
);
if ( ! is_readable( __DIR__ . '/vendor/autoload.php' ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In the event further composer dependencies are added at a later date includingthis here may cause problems if the new dependencies require > PHP 5.6

Would it be possible to do one of the following:

  • only require the compatibility checker in the vendor path
  • install the compatibility checker outside of vendor and require it from the custom location

Copy link
Member Author

Choose a reason for hiding this comment

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

@peterwilsoncc I've taken the approach to move the library from vendor to 10up-lib directory and also loaded the SPO main class after the validation is completed.

Copy link
Member

@iamdharmesh iamdharmesh left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @Sidsector9. Code LGTM! and it tests well (Didn't throw any fatal error on 5.6 now).

In release build zip I have noticed some .md files in 10up-lib folder but it doesn't seems a blocker here. So, we are good to go here.
Screenshot 2023-10-09 at 10 38 19 PM

Thank you.

@Sidsector9
Copy link
Member Author

Thanks for testing @iamdharmesh
The files you're seeing is expected as you can also see them in the vendors directory.
I'll resolve this in the next release of WP Compat Validator Tool.

@Sidsector9 Sidsector9 merged commit 51d1cbe into develop Oct 10, 2023
@Sidsector9 Sidsector9 deleted the upkeep/163 branch October 10, 2023 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:code-review This requires code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running PHP 5.6 the plugin fatals before the PHP requirements check runs
3 participants