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

Issue/46 - Fix magic methods not working as intended #47

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

JJJ
Copy link
Collaborator

@JJJ JJJ commented May 6, 2020

This pull request changes quite a bit about how classes store their properties.

  • Everything gets stored in a $this->data array
  • Magic methods in Base have internal logic for allowing get_ functions to work as intended
  • Any variable that tries to get set to $this->foo ends up in $this->data['foo'] so that it can always be magically overridden, or not

All of this is required to make overrides work as intended. But then, WordPress functions that use WP_List_Util will then stop working, because they use array_key_exists() which will bypass PHP magic methods completely.

That means this PR either requires:

  • an upstream fix to WordPress Core
  • shimming that utility internally so that it does not require WordPress in this situation

Fixes #46.

Related to: https://core.trac.wordpress.org/ticket/50095

JJJ added 2 commits May 5, 2020 21:45
This commit introduces a private data() array, and a public method for classes that extend Base to call to setup the defaults.

This is necessary to make sure that all default class variables make their way into the data array so that magic methods can access them like they can any other variable.
This commit is necessary to make sure that all object properties are correctly setup in the private data array.
@JJJ
Copy link
Collaborator Author

JJJ commented Aug 25, 2020

The above upstream WordPress core change was merged into WordPress 5.5!

@alexstandiford
Copy link
Collaborator

So should we assume that anything using BerlinDB should be on 5.5 or greater?

@JJJ
Copy link
Collaborator Author

JJJ commented Aug 26, 2020

So should we assume that anything using BerlinDB should be on 5.5 or greater?

Once this is merged, yes.

* Change from array to object
* Introduce unset_vars() to parlay off of set_vars()
* Introduce validate_key() to reduce some code duplication in magic methods
* Switch from is_callable() to method_exists() so that it works as intended with __call()
* Make sure magic un/set methods operate on the correct variables
* Avoid setting object array keys directly, triggering byref debug notices
* Some general code & docs improvements to related methods
* Add 'join' to query & request clauses
Base automatically changed from release/1.1.0 to master February 23, 2021 05:59
Copy link
Collaborator

@alexstandiford alexstandiford left a comment

Choose a reason for hiding this comment

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

Just adding a note that it would be a good idea to add a note in the Readme that BerlinDB currently requires a minimum of WordPress 5.5. This may be a detriment to some plugins that want to be compatible further than that.

@JJJ
Copy link
Collaborator Author

JJJ commented Sep 8, 2021

add a note in the Readme

👍

BerlinDB currently requires a minimum of WordPress 5.5

It doesn't yet, but it will when #46 is resolved, which is in the Ongoing milestone for pretty much this reason.

Based on the WordPress functions that Berlin uses, its minimum is probably 3.3 thanks to it calling wp_suspend_cache_addition() in Query::cache_set().

That's a pretty big jump from 3.3 to 5.5, so I'm not super eager to merge this. 😬

@alexstandiford
Copy link
Collaborator

Yep, totally understandable.

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.

Magic functions starting with "get_" are not working as intended
2 participants