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

largo_remove_hero does not remove duplicate heroes on Gutenberg content. #1834

Open
benlk opened this issue Nov 14, 2019 · 2 comments
Open
Labels
category: gutenberg Relating to general Gutenberg compatibility priority: normal Must be completed before release of this version of plugin. type: bug type: question type: tech debt For necessary updates because code is old

Comments

@benlk
Copy link
Collaborator

benlk commented Nov 14, 2019

https://github.com/INN/largo/blob/512da701664b329f2f92244bbe54880a6e146431/inc/post-templates.php#L191-L204

In situations where the first block on a post is an image block, the first paragraph of the post is not the image tag. It's a comment.

<!-- wp:image {"id":4923609,"className":"size-large wp-image-4923609"} -->
<figure class="wp-block-image size-large wp-image-4923609"><img src="https://citylimits.test/wp-content/uploads/2019/06/9351636396_89cf3d92d0_o-771x514.jpg" alt="sex work protest" class="wp-image-4923609"/><figcaption><p class="wp-media-credit"><a href="https://www.flickr.com/photos/piczilla/9351636396/in/photolist-ffnzGh-ikZTmj-csDva7-ffnA5o-csDoqd-wXnJH9-72Zn35-csDq8d-4ngpvq-csDgAh-cE31Ff-csD5ym-csDdhs-5QrDsM-csDeZm-csDnrE-bjLsQR-7hYi6X-29vuZow-csD7A7-SSHNLZ-5Ksxb2-4ViLfG-csDrR7-bku5n4-bjsjfD-bku4jc-bjLATr-bku6Qr-hc1G6T-6Vm8KP-bjLzin-bjseHF-bjLrbk-bjLvVe-bku6aM-bjsaka-hc33Hk-bkueEX-hc35hx-6VrNoo-bjLiHz-bjsdKe-92JTvN-4ZUf7d-92JN2Q-KTUb38-92JGbC-92JBYU-6VmJQK">Type 17 Productions</a></p>Sex workers rally in 2013. Some sex workers argue that policies aimed at 'ending demand' actually increase the risks they face.</figcaption></figure>
<!-- /wp:image -->

as a post content will be passed through the filter.

$p[0] is '<!-- wp:image {"id":4923609,"className":"size-large wp-image-4923609"} -->', not an image that matches the regex.

Even if we adjusted this algorithm to skip the opening comment, and to treat figcaptions like the paragraph tag, we're edging into the dangerous territory for parsing HTML with regular expressions. https://stackoverflow.com/a/1732454

@benlk benlk added type: bug type: question priority: normal Must be completed before release of this version of plugin. type: tech debt For necessary updates because code is old category: gutenberg Relating to general Gutenberg compatibility labels Nov 14, 2019
@benlk
Copy link
Collaborator Author

benlk commented Nov 14, 2019

If we want to fix this parser:

  • we're going to have to switch to DomDocument or something similar to parse the post. This will add page load time

If we don't want to fix the parser:

  • We're going to need to stop outputting featured media on posts.
  • Featured media will have to be presented on a post by putting it in the post content, whether that be in a block or some other method.
  • The post thumbnail functionality will not be affected.
  • We'll need to remove all Largo Featured Media code and functionality from the theme (or leave the functions in the code, with deprecation warnings for 0.8)
  • This will be a change that affects a lot of legacy content; we'll need to communicate it with sites.
  • For compatibility with old content, we may want to implement a shim that:
    • when the Largo update function is run, saves the datetime to the DB
    • on posts created/saved before the update date, outputs largo_hero via that function
    • provides a toggle in the admin to control whether that legacy Featured Media shim is used

@benlk
Copy link
Collaborator Author

benlk commented Nov 14, 2019

Personally, I'm in favor of the deprecation technique.

By removing Largo Featured Media in favor of Gutenberg, sites will have more editorial control over how the media displays in their posts, with the same level of control over whether it displays at all. Getting rid of the featured media output would also bring Largo's existing templates closer to the "Blank Slate" template described in #1663

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: gutenberg Relating to general Gutenberg compatibility priority: normal Must be completed before release of this version of plugin. type: bug type: question type: tech debt For necessary updates because code is old
Projects
None yet
Development

No branches or pull requests

1 participant