Skip to content

Ensure post-featured-image block is in_the_loop() for BC with core and plugins, and to fix lazy-loading #45534

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 16, 2022
Merged

Ensure post-featured-image block is in_the_loop() for BC with core and plugins, and to fix lazy-loading #45534

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/block-library/src/post-featured-image/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@ function render_block_core_post_featured_image( $attributes, $content, $block )
}
$post_ID = $block->context['postId'];

// Check is needed for backward compatibility with third-party plugins
// that might rely on the `in_the_loop` check; calling `the_post` sets it to true.
if ( ! in_the_loop() && have_posts() ) {
the_post();
}

$is_link = isset( $attributes['isLink'] ) && $attributes['isLink'];
$size_slug = isset( $attributes['sizeSlug'] ) ? $attributes['sizeSlug'] : 'post-thumbnail';
$post_title = trim( strip_tags( get_the_title( $post_ID ) ) );
Copy link
Member

@Mamaduka Mamaduka Nov 16, 2022

Choose a reason for hiding this comment

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

Should we remove the $post_ID usage from the context now that we set the global post object? I think we did something similar in #37622.

Copy link
Member Author

@felixarntz felixarntz Nov 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@Mamaduka Oh you mean not pass $post_ID to these functions and instead rely on the default which would be the same current post anyway? That seems reasonable to me.

We could do this here and also further down on get_the_post_thumbnail() and get_the_permalink().

Do you think I should add it to this PR or rather in a separate PR?

Copy link
Member

@Mamaduka Mamaduka Nov 16, 2022

Choose a reason for hiding this comment

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

Oh you mean not pass $post_ID to these functions and instead rely on the default which would be the same current post anyway? That seems reasonable to me.
We could do this here and also further down on get_the_post_thumbnail() and get_the_permalink().

Correct.

Do you think I should add it to this PR or rather in a separate PR?

We can do it in a separate PR if you prefer.

Copy link
Member Author

@felixarntz felixarntz Nov 16, 2022

Choose a reason for hiding this comment

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

@Mamaduka Sounds good, I'll open a follow up PR once this is merged. Is this waiting for anything else or should we merge it?

Copy link
Member

@Mamaduka Mamaduka Nov 16, 2022

Choose a reason for hiding this comment

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

Feel free to merge 👍

Copy link
Member Author

@felixarntz felixarntz Nov 17, 2022

Choose a reason for hiding this comment

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

Yeah, that's the main reason I'm asking. Is there any way we can cherry-pick / backport this change to core sooner? Happy to help as much as I can.

Copy link
Member

@Mamaduka Mamaduka Nov 17, 2022

Choose a reason for hiding this comment

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

We'll have to do a minor release for the packages outside the release scope, but I don't know what's the policy here. I'm going to ping folks who have done more release than me.

Cc. @noisysocks, @youknowriad

Copy link
Contributor

@youknowriad youknowriad Nov 18, 2022

Choose a reason for hiding this comment

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

We'll have to do a minor release for the packages outside the release scope, but I don't know what's the policy here. I'm going to ping folks who have done more release than me.

This is something we do often I think. We'll have to cherry-pick the commits to the right wp/* branch and perform a npm patch release from there. I'm not entirely certain with the process (as it changed lately a bit) but I know that it's documented.

Copy link
Member

@Mamaduka Mamaduka Nov 18, 2022

Choose a reason for hiding this comment

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

Thanks, Riad! I'm familiar with the new process and can do a patch release.

@felixarntz, I can do a package release and prepare core PR early next week.

Copy link
Member Author

@felixarntz felixarntz Nov 18, 2022

Choose a reason for hiding this comment

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

Perfect, thanks both! Let me know if I can help with anything like reviews or so.

Expand Down