Remove globally defined $post variable

[dingo-d]

While reviewing a PR I've noticed this part

eightshift-frontend-libs/blocks/init/src/Blocks/custom/featured-posts/featured-posts.php

Lines 20 to 23 in beb0937

	global $post;
	?>

In the featured posts block.

I'm not sure what is this doing here tbh. @moonbyt3 suggested that this can be used on a single page, probably when we want to exclude the current post from the related posts

if ($featuredPostsExcludeCurrentPost) {
	args['post__not_in'] = [$post->ID];
}

But this is not something we should do in the first place.

So I think this should be removed. The logic for querying posts is usually added via filter IIRC inside a special class, so all the querying and PHP filtering can be done that way.

@infinum/wordpress-team thoughts, suggestions, well wishes? 😄

[MetarDev]

But this is not something we should do in the first place.

It gets much harder to exclude current post this using post_not_in argument (you have to fetch n+1 and then filter it out IF you got the current post back from the query, if not then you have to stop before the last one or you will show 1 extra post). So that small performance penalty is fine imo. And even then you somehow need to know the current post ID.

If you're more worried about using a global variable inside a block, we could just use get_the_ID().

Dunno I'd leave it as it is, don't think it's too big of a deal to have a global var in and it makes the implementation of excluding current post feature simpler.

[dingo-d]

I'd like to investigate the behavior of this global object here. In general we should definitely avoid using globals as much as possible (you never know what you may get, or if the global post can be changed and affect the results somehow).

As for complexity, the example in the link doesn't seem to be that complex, and you can just pass the current ID when building the custom query and rendering related posts.

Not sure what the performance penalty is, I'll try to play a bit and see the results using query monitor 👍🏼

EDIT: A more in-depth explanation of why post__not_in should be avoided: https://docs.wpvip.com/technical-references/code-quality-and-best-practices/using-post__not_in/

[goranalkovic-infinum]

@infinum/wordpress-team what's the final decision for this?

Do we keep the global $post?

Do we drop the post_not_in and fetch n + 1 posts, then check if the current one is in there and take it out, otherwise, render first n posts? Do we just leave it be?

[dingo-d]

If it's a big change that would take a couple of hours/days to implement I'd say, for now, leave it as is.

We can optimize this in the projects later on. And I don't think that it's a BC break even if we change it down the line, no?

I'd definitely like it to be as performant as possible.