Add support for image blocks

[jorostoyanov]

Overview

This PR is a continuation of #475.

It adds the removed support for image blocks parsing.

The logic is to loop through each image block of a Post and test if it can be used as a Featured Image. If such blocks don't exist or the images inside are not an actual Attachment, continue with the raw post content parsing.

---

• confirmed that PHPCS has been run

[iuravic]

Here's a code readability suggestion. Writing a block of code and then adding comment after the block makes it difficult to read and understand. I'd suggest one of these two improvements:

1. good -- move your comments to the top of your code, and instead of "Reaching here...", just use a comment like "// Check if post_content contains an image block with an 'id' attribute."
2. better -- put that block in a method which has such a nice name that it's practically self-documenting, e.g. public function does_content_have_img_block_with_id_attr( string $post_content ): bool

Which ever you prefer personally, both are fine. I like 2. but no need to do it if you're more comfortable w/ 1.

But even more importantly, $has_set_featured_image_from_blocks has been set, but post has not actually been updated in that last if-block you wrote. Am I missing something here? Sorry if I am. But it looks like you detected that there is an image block with attribute, but have not used that info anywhere after that.

[jorostoyanov]

Hey @iuravic 👋

Thank you for the review! 🙇

I will go and improve the documentation part. While I was writing it, I thought it was better to add the explanation after the if-block, to explain why the suggested else statement is not used and won't be needed.

I will not update it, since we can agree that if-else won't be a suitable refactoring here 🤝

But even more importantly, $has_set_featured_image_from_blocks has been set, but post has not actually been updated in that last if-block you wrote. Am I missing something here? Sorry if I am. But it looks like you detected that there is an image block with attribute, but have not used that info anywhere after that.

Regarding this — it might be not so obvious where am I setting the Featured Image (https://github.com/Automattic/newspack-custom-content-migrator/pull/488/files#diff-6301904e452902f298dc69e35a450fb7e4a9f7a8d6376030dd0916d9f4d352afR430).

Let me know if you missed it or if I should refactor it a bit. Or, if I didn't understand your comment and you meant something else 😅

[iuravic]

Excellent, thanks @jorostoyanov, updating the comments will be great.

As for the line which sets the image, sorry I missed that part. The following comment purely a style point, so it's not a requirement for the PR, just a bit of discussion, I will leave it up to you to decide which of these two you'd like to use/keep.

Current:

if ( set_post_thumbnail( $post_id, $image_block['attrs']['id'] ) ) {
	$has_set_featured_image_from_blocks = true;
	break;
} else {
	WP_CLI::line( sprintf( '❌ Could not set Featured Image from Block. ID %d', $image_block['attrs']['id'] ) );
}

What I personally would propose, just from my own coding style perspective. This one looks more readable ti me, I can understand it in less time.

$has_set_featured_image_from_blocks = set_post_thumbnail( $post_id, $image_block['attrs']['id'] ) ? true : false;
if ( false === $has_set_featured_image_from_blocks ) {
	WP_CLI::line( sprintf( '❌ Could not set Featured Image from Block. ID %d', $image_block['attrs']['id'] ) );
}

But the this style choice is about you. What you think is nicer.

Once you've updated the comments, and if you decide to update this part, feel free to merge to master 👍 That's why I'm approving now, early.