themehybrid / hybrid-core

Official repository for the Hybrid Core WordPress development framework.
GNU General Public License v2.0
687 stars 143 forks source link

5.0: `Attr::post()` should accept `$post` arg, for simpler usage outside the loop #171

Closed lkraav closed 4 days ago

lkraav commented 5 years ago

Codebase grep shows only Attr::post() has been left out of this argument club:

src/Attr/Attr.php|269 col 12| $post  = get_post();
src/Media/Meta.php|57 col 17| $this->post = get_post( $post );
src/Post/functions-post.php|441 col 10| $post = get_post( $post );
src/Post/functions-post.php|456 col 12| $post   = get_post( $post );
src/functions-filters.php|747 col 13| $post    = get_post( $post_id );
src/functions-filters.php|879 col 11| $post = get_post( $post_id );

setup_postdata() is fragile and inconvenient in nested loops, where I'd love to use \Hybrid\Attr\display().

justintadlock commented 5 years ago

Adding a $post parameter will not make it usable outside of The Loop. To do that, it'd require either an extra parameter in __construct() or to sort of shoehorn $context into using a post ID.

This will be a bit more involved than what the PR handles. I'll think on it.


setup_postdata() should be a non-issue in nested loops if you're using it along with wp_reset_postdata(). You don't even have to call it directly if you use WP_Query().

<?php while ( have_posts() ) : the_post(); ?>

    <?php $loop = new WP_Query(); ?>

    <?php while ( $loop->have_posts() ) : $loop->the_post(); ?>

    <?php endwhile; ?>

    <?php wp_reset_postdata(); ?>

<?php endwhile; ?>
lkraav commented 5 years ago

Depending on what the external plugins do, I may only have an array of posts available to work with, so no custom WP_Query is created. It'd be unoptimal to start rewriting and taking on maintenance of outside querying logic across plugins.

It'd be super nice if I could tell Attr what post object to fetch information from. It looks simple enough on surface, I'll take a closer look at your reply's logic in a bit.

justintadlock commented 5 years ago

Even with an array of posts, you're only looking at:

<?php foreach ( $posts as $post ) : ?>

    <?php setup_postdata( $post ); ?>

<?php endforeach; ?>

<?php wp_reset_postdata(); ?>

My preference would be for Attr to be completely agnostic of post IDs and such and just be a general-purpose class for attributes. A post attribute sub-class may even be something better suited for this.

lkraav commented 5 years ago

Even with an array of posts, you're only looking at

It's not too "only", there are many things to know.

Can't forget declaring global $post, or things won't work as expected

Nested loops with a single global $post variables doesn't seem to work

<?php global $post; ?>

<?php foreach ( $tracks as $key => $post ) : ?>
    <?php setup_postdata( $post ); ?>
    ... still displays correct data at top nesting level

    <?php foreach ( $track->courses as $key => $post ) : ?>
        <?php setup_postdata( $post ); ?>
        ...FAILS at this nesting level

Have to do "convention" dances for things to work properly, like:

<?php global $post; ?>

<?php foreach ( $tracks as $key => $track ) : ?>
    <?php $post = $track; ?>
    <?php setup_postdata( $post ); ?>
    ...

    <?php foreach ( $track->courses as $key => $course ) : ?>
        <?php $post = $course ?>
        <?php setup_postdata( $course ); ?>
        ...

Seems like some kind of a global variable clobbering vs foreach scopes issue. I should Xdebug it one of these days to understand.

saas786 commented 3 years ago

@lkraav is this related to: https://github.com/themehybrid/hybrid-core/pull/172 ? If so, I think we can close it too?

saas786 commented 4 days ago

@lkraav

Please let me know if you still require assistance with this. I will go ahead and close it for now.

lkraav commented 4 days ago

@lkraav

Please let me know if you still require assistance with this. I will go ahead and close it for now.

Haven't thought about it for a while, thanks.