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

Explicitly declare global #34

Merged
merged 2 commits into from
Oct 21, 2019

Conversation

pdewouters
Copy link
Contributor

@pdewouters pdewouters commented Oct 11, 2019

Description of the Change

Fixes an error related to variable scope

Uncaught Error: Call to a member function get_avatar() on null in /chassis/vendor/10up/simple-local-avatars/simple-local-avatars.php on line 580get_simple_local_avatar()vendor/10up/simple-local-avatars/simple-local-avatars.php:254edit_user_profile()wp-includes/class-wp-hook.php:286apply_filters()wp-includes/class-wp-hook.php:310do_action()wp-includes/plugin.php:465do_action()wp-admin/user-edit.php:700require_once()wp-admin/profile.php:18

Benefits

The plugin can be included via our loading mechanism

Possible Drawbacks

Verification Process

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.

Applicable Issues

Changelog Entry

Initialize Simple_Local_Avatars on the $simple_local_avatars global, enabling bundling plugin with composer.

@jeffpaul jeffpaul added this to the 2.2 milestone Oct 11, 2019
Copy link

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

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

Hi @pdewouters and thanks for the PR!

Can you give some more details about why you want to make this change?

  • What exact conditions are used to create the error you show above, can you share your loading code?
  • Have you considered other approaches?
  • Could this change have unintended side effects?

@pdewouters
Copy link
Contributor Author

@adamsilverstein yes here's where the plugin is loaded: https://github.com/humanmade/altis-cms/pull/111/files , we want to include it via composer, and then load it on the plugins_loaded action by including the plugin file manually.
The reason this error occurs is because $simple_local_avatars is not in the global scope when the plugin file is included via a calling function.

Other approach could be:

global $simple_local_avatars;
$simple_local_avatars = new Simple_Local_Avatars();

Instead of making it a super global with $GLOBALS - this might be preferable?

@adamsilverstein
Copy link

@pdewouters thanks for clarifying, i'll take a closer look and let you know what we can do here.

@adamsilverstein
Copy link

@pdewouters - this looks good, i verified the issue and the fix.

can you please change to your suggested

global $simple_local_avatars;
$simple_local_avatars = new Simple_Local_Avatars();

to better match the rest of the code

Co-Authored-By: Adam Silverstein <[email protected]>
Copy link

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

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

Thanks @pdewouters!

@adamsilverstein adamsilverstein merged commit 42c41b3 into 10up:develop Oct 21, 2019
@pdewouters
Copy link
Contributor Author

Thanks for merging @adamsilverstein , any plans for a new tagged release?

@jeffpaul
Copy link
Member

jeffpaul commented Jul 1, 2020

@pdewouters per #51, we're looking to release 2.2.0 sometime in July. We've got 2 releases ahead of it on other open source projects, but hope to get to 2.2.0 soon.

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.

3 participants