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

Feature/block filters #432

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
Open

Feature/block filters #432

wants to merge 13 commits into from

Conversation

cmmarslender
Copy link
Contributor

@cmmarslender cmmarslender commented Jul 17, 2019

Description of the Change

On distribution, converts block comments to a DOMDocument-friendly format, then parses the html and iterates, firing a filter on each block that is then encountered. After all filters are run, it is converted back to normal gutenberg comments and then saved.

Alternate Designs

Could possibly try to just parse the gutenberg "markup" without DOMDocument, but probably safer to use a library that is designed for this, since its relatively easy to convert the gutenberg comments into something that looks like html that DOMDocument will parse

Benefits

Allows filtering block attributes before the post is distributed.

Possible Drawbacks

Verification Process

Tested the output of the conversion functions and am using this filter on a current project to ensure media + taxonomy IDs are translated as part of pushing content to another site

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

TODO:

  • Unsure off hand if DOMDocument is included with php or a module. Should check for this and adjust code accordingly
  • mb_convert_encoding requires mbstring module. Should ensure this is available before using the function

Applicable Issues

#430

@jeffpaul jeffpaul added the type:enhancement New feature or request. label Jul 17, 2019
@adamsilverstein
Copy link

very interesting! I was going to suggest considering core's block parser however you rightly pointed out that it doesn't support a way to reverse the parse process once you modify the attributes. Apparently that is handled entirely in JavaScript.

https://github.com/WordPress/gutenberg/blob/master/packages/block-serialization-default-parser/parser.php

@cmmarslender
Copy link
Contributor Author

Yep.. I initially looked at using the core parser to parse them and then reassembling that data myself, but as far as I could tell, there wasn't any way to figure out where within the markup we should put back innerBlocks, etc. Annnnd thats how I ended up with the current approach :)

@jeffpaul jeffpaul added this to the Future Release milestone Dec 18, 2019
@jeffpaul
Copy link
Member

jeffpaul commented Jan 3, 2020

@cmmarslender @adamsilverstein I wanted to check in to see if either of you had plans to put further work into this draft PR? Either way, happy new year and I hope you both have a safe, happy, and prosperous 2020... cheers!

@adamsilverstein
Copy link

👋 Hey @jeffpaul - thanks for the ping. I doubt I will have time to revisit this in the near future - probably best tackled by someone working more closely on Distributor.

@gthayer
Copy link

gthayer commented Jan 17, 2020

I've taken over the project this CR originally came from and ran into a bug where there was an incorrect space being added to the Block, preventing it from being read correctly on the Distributed site. Submitted a patch here: 62c2ae5

@gthayer
Copy link

gthayer commented Mar 4, 2020

@jeffpaul - I've added the check for DOMDocument, though I'm not sure what to do about about CIs failing here. Distributions are working for me locally.

@gthayer
Copy link

gthayer commented Mar 4, 2020

I'm seeing other PRs will the same issue, so maybe its a problem with the test itself?

@jeffpaul
Copy link
Member

jeffpaul commented Mar 6, 2020

@gthayer aside from the tests, are there any further enhancements/fixes you have pending on this PR or is it otherwise ready for review?

@gthayer
Copy link

gthayer commented Mar 6, 2020 via email

@jeffpaul
Copy link
Member

jeffpaul commented Mar 6, 2020

@gthayer wonderful, thanks!

@dinhtungdu are the failing tests here the same that you're helping to triage or something else that should give us concern in reviewing/merging this PR?

@dinhtungdu
Copy link
Contributor

@jeffpaul Yes, the failing one is author test, which is not wpsnapshots butadmin. I fixed it in other PR, if you think a separate PR to fix it is better, I can submit one.

@jeffpaul
Copy link
Member

jeffpaul commented Mar 7, 2020

@dinhtungdu nope, we're fine how it is then.

@jeffpaul jeffpaul marked this pull request as ready for review March 7, 2020 01:44
@jeffpaul jeffpaul modified the milestones: Future Release, 2.0.0 Mar 7, 2020
@jeffpaul
Copy link
Member

There's still a good amount we need to discuss on this and it relates to work we want to tackle in 2.1.0, so I'm going to move this to the 2.1.0 release in hopes someone's able to pick this up and own it during that release cycle.

@jeffpaul jeffpaul modified the milestones: 2.0.0, 2.1.0 Mar 26, 2020
@jeffpaul jeffpaul added the needs:discussion This requires discussion to determine next steps. label Mar 26, 2020
@jeffpaul jeffpaul removed the request for review from helen January 24, 2022 17:48
@dkotter dkotter removed the request for review from dinhtungdu August 23, 2022 22:12
@jeffpaul jeffpaul removed the request for review from dkotter January 16, 2023 18:01
@jeffpaul jeffpaul linked an issue Jan 16, 2023 that may be closed by this pull request
@jeffpaul
Copy link
Member

Unassigning reviewers here while we focus on the v2 refactoring and can then reassess how best to handle the root issue here.

@github-actions github-actions bot added the needs:refresh This requires a refreshed PR to resolve. label Feb 9, 2024
Copy link

github-actions bot commented Feb 9, 2024

@cmmarslender thanks for the PR! Could you please rebase your PR on top of the latest changes in the base branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:discussion This requires discussion to determine next steps. needs:refresh This requires a refreshed PR to resolve. type:enhancement New feature or request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Distributing blocks with site-specific attributes doesn't work
6 participants