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

Do not allow the usage of magic properties on WP_Post objects #229

Open
fklein-lu opened this issue Oct 7, 2020 · 4 comments
Open

Do not allow the usage of magic properties on WP_Post objects #229

fklein-lu opened this issue Oct 7, 2020 · 4 comments
Labels

Comments

@fklein-lu
Copy link

The WP_Post class offers access to various properties through magic getters:

  • $page_template
  • $ancestors
  • $post_category
  • $tag_input

In addition, post meta can be retrieved by using $post->meta_key.

We should not allow the usage of these coding approaches:

  1. Magic properties are discouraged in the PHP community, because IDEs and other code quality tools cannot understand them.
  2. Since there are reserved keywords, using $post->meta_key does not always return the meta data.
  3. Depending on how the post is instantiated, the meta value will be passed through sanitize_post_field() before being returned.
  4. Meta will always be pulled with $single set to true.

All in all there are no benefits here, and only downsides.

@tfrommen tfrommen added the phpcs label Oct 19, 2020
@goldenapples
Copy link

Regarding the concern about IDEs and code analyzers, there is a PHPDoc syntax (@property, or @property-read for properties that are read-only) for documenting properties which will be accessed through magic getters. I believe it would look something like this:

<?php
/**
 * Post object.
 *
 * @var WP_Post
 * @property-read string $subtitle Post subtitle.
 */
 $post = get_post();

 ?>
<h2><?php the_title() ;?></h2>
<p><?php echo esc_html( $post->subtitle ); ?></p>

... which actually offers more documentation and type hinting than inline get_post_meta() calls would.

What are the main arguments against magic properties in the PHP community? Did a bit of searching and I didn't see anything written about it.

Personally, I think allowing access to post properties through magic getters in specific contexts (like template files) allows for readable and expressive code, and the ergonomic savings of not writing out get_post_meta() over and over is a big bonus. I'm happy to avoid this syntax if there's a consensus against it, but I'm not seeing the downsides here. Newly-learned syntactic sugars like $post->meta_key seem obscure or "clever" when you're first exposed to them, but I think this might be a case where its worth exploring allowing them.

@fklein-lu
Copy link
Author

Regarding the concern about IDEs and code analyzers, there is a PHPDoc syntax (@property, or @property-read for properties that are read-only) for documenting properties which will be accessed through magic getters.

Where would this documentation be added? For the properties that are part of the WP_Post class, the Core code would need to be changed. Not impossible, but would need patches, and buy-in from the Core team.

This wouldn't cover post meta though, since the WP_Post class cannot know about which meta fields a post has. So that leaves documentation on the retrieved object, as shown above. Which is not practical for two reasons.

First once you retrieve the same post in two different places, you would have to copy over the documentation.

Secondly once a new meta field is added, one would need to ensure that the documentation is updated. Which is hard to do even if there is just a single instance. Because developers first would need to know that they have to do this, and they would need to know where to do this.

.. which actually offers more documentation and type hinting than inline get_post_meta() calls would.

Not sure about the type hinting, since a meta value retrieved with the $single argument set to true is always either an array or a string. The type therefore in most cases does not express what it should be. A boolean for example could be stored as '1' or '0', or if developers are doing it wrong 'on' or 'off'.

Personally, I think allowing access to post properties through magic getters in specific contexts (like template files) allows for readable and expressive code, and the ergonomic savings of not writing out get_post_meta() over and over is a big bonus.

Due to the limitations of PHPCS, it's hard to restrict rules to certain type of files. So it's either allowed everywhere, or not allowed everywhere.

Newly-learned syntactic sugars like $post->meta_key seem obscure or "clever" when you're first exposed to them, but I think this might be a case where its worth exploring allowing them.

WordPress has implemented these magic getters because its classes don't use object oriented programming principles. Getters and setters have always been the norm in PHP, although there are exceptions of course.

I agree that getter method calls are more expressive and readable, but using magic getters is using the wrong tool for the job. If you want to have a nicer interface, with type hinting benefits, then use business objects.

These are classes that tie together custom post types, post meta, terms, etc. into a consistent interface that speaks business language, rather than low level implementation language. Such an approach was implemented on TechCrunch, see the Event class, and the associated factory for such types.

With the new more detailed documentation for arrays, and argument and return type hinting, this can be really powerful.

@svandragt
Copy link
Contributor

svandragt commented Oct 29, 2020

I agree with Fränk main problem for me is that it's bypassing filters. For example: the "default_{$meta_type}_metadata" filter for post meta. Plugins and other first/third party code rely on these filters being in place and bypassing them therefore should be discouraged in my view. The result is unreliable.

If you need to have the direct access then there are ways to silence the coding standards but the default way should be to use the filter points.

The other arguments are less relevant than that.

@goldenapples
Copy link

Just for clarity, I don't think getting meta values through the magic getter bypasses any filters - it's just a syntactic sugar for calling get_post_meta(), so it hits the meta cache and the get_post_metadata and default_post_metadata filters just like accessing meta in any other way would.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants