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

Ensure we load our autoloader from our plugin directory #231

Merged
merged 2 commits into from
Dec 7, 2022

Conversation

dkotter
Copy link
Collaborator

@dkotter dkotter commented Dec 7, 2022

Description of the Change

In #217 a new composer dependency was added and to ensure it gets loaded, a composer autoloader was required. This was loaded with a require_once statement that just referenced the vendor directory.

The problem here is that we weren't specifically referencing the directory we should load from. This can result in the autoloader being loaded from somewhere else. For instance, if you have an autoloader at the root of your site (say you use composer to manage all your plugins), this autoloader will be loaded instead in certain situations, causing fatal errors since the dependency we expect is now not loaded.

This PR fixes this by adding the __DIR__ constant before the file path, ensuring the autoloader is loaded from the root directory of this plugin. We also output an admin notice if that autoloader isn't found and if the base Factory class doesn't exist. This should only be those that are running this plugin directly from GitHub and need to run composer install

Alternate Designs

Could use plugin_dir_path( __FILE__ ) instead of __DIR__ but they both return the same value and my preference is the latter.

Benefits

The correct autoloader is always used

Possible Drawbacks

None

Verification Process

Before checking this PR out, add a composer autoloader to the root of your WordPress install (something like wordpress/vendor/autoload.php).

Setup Restricted Site Access, add one dummy IP address that is allowed and then try and access the site from an incognito window (seems like the issue happens only if you're not logged into WordPress).

You should get a fatal error.

Then check out the code from this PR and run the same tests again.

Things should work now

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Changelog Entry

Fixed - Ensure we load our autoloader from the root of our plugin directory
Added - Show an admin notice if our autoloader doesn't exist

Credits

Props @dkotter, @pablojmarti, @shahzaib10up

Applicable Issues

Closes #229

@dkotter dkotter self-assigned this Dec 7, 2022
@dkotter dkotter requested a review from Sidsector9 December 7, 2022 21:48
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

LGTM.

Is it possible to do something like 10up/safe-svg#52 to account for people loading the plugin via composer?

I know we don't have it it packagist but it's easy enough to refer to a github repo in a top level composer file.

@jeffpaul jeffpaul added this to the 7.4.0 milestone Dec 7, 2022
…ngs aren't setup, for those that are installing the plugin from GitHub. Also account for Packagist installs
@dkotter dkotter requested a review from peterwilsoncc December 7, 2022 22:20
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

Thank you kindly.

@peterwilsoncc peterwilsoncc merged commit c6e870b into develop Dec 7, 2022
@peterwilsoncc peterwilsoncc deleted the fix/autoload-require branch December 7, 2022 22:32
@dkotter dkotter modified the milestones: 7.4.0, 7.3.5 Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PHP Fatal Error when using IP restriction
3 participants