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

content filtering conditions #1539

Merged
merged 12 commits into from
Feb 22, 2021
Merged

content filtering conditions #1539

merged 12 commits into from
Feb 22, 2021

Conversation

thomasplevy
Copy link
Contributor

@thomasplevy thomasplevy commented Feb 20, 2021

Description

I think this is a descent approach to fixing the issue(s) we've been encountering as a result of recent changes introduced into Yoast 15.8.

The changes which have caused these errors to start appearing were introduced in 15.8, specifically Yoast/wordpress-seo@9b4f7ca which loads the $post global.

Previously, since the global wasn't setup in the conditions where we're now encountering issues (the course builder and during cloning / importing on the admin panel) we didn't have any problems. But now that the $post global exists on sites running Yoast (it looks like this is loaded on the admin panel by Yoast intentionally but I can't tell exactly why, nor have I tried to figure it out).

This PR introduces conditional loading of our filters so that on the admin panel (generally, even though we care specifically about the course builder and course importer / cloninig function) and during REST requests the templates and restriction logic handled by llms_get_post_content() isn't executed.

This is a WIP. I haven't finished testing it and probably even writing it. I'm hoping to get this deployed this weekend because Yoast conflict bug reports are coming in regularly since 15.8 dropped and the severity is raising for us as a result.

Fixes #1536
Fixes #1530

This should also fix https://github.com/gocodebox/lifterlms-assignments/issues/55 (but I haven't run a test against this yet, just theoretically)

How has this been tested?

  • Added new unit tests, needs more thorough testing to actually ensure the conflict with Yoast (and theoretical conflict with other plugins) doesn't happen anymore. I think this will all fix it based on @eri-trabiccolo recommendations across the various related issues and (failed) PRs to fix the issue.

Screenshots

Types of changes

  • bug fix

Checklist:

  • My code has been tested.
  • My code passes all existing automated tests.
  • My code follows the LifterLMS Coding & Documentation Standards.

@eri-trabiccolo
Copy link
Collaborator

@thomasplevy
While I see your reasons here I'm not sure this is the best approach.
At least it's not so for the REST api when returning the llms_post content "rendered", if it's not built with the block editor. In the content rendered, in fact, we display the post as it appears in front, so, to say, for a course, it'll display the instructors, the pricing table etc... which when using the block editor they're just blocks, but when not using it they are hooked to action hooks fired in templates loaded by llms_get_post_content.
Also content restrictions which do not imply a redirect are handled there too.

If we decide this is the way to go, we need to add back this filter, e.g. here:
https://github.com/gocodebox/lifterlms-rest/blob/1.0.0-beta.18/includes/abstracts/class-llms-rest-posts-controller.php#L835

Right?

Because ideally, we should skip the content filtering only when inserting (updating) posts either in admin or REST requests: skip when inserting, but not when returning the inserted one.

Also, while this fixes all the issues you cited, in the assignment plugin we use a similar filter, which anyways doesn't produce any issues because the template it loads are fine :D

Copy link
Collaborator

@eri-trabiccolo eri-trabiccolo left a comment

Choose a reason for hiding this comment

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

only a doc to be added.
And a consideration...

includes/admin/class.llms.admin.builder.php Show resolved Hide resolved
includes/functions/llms-functions-content.php Show resolved Hide resolved
@thomasplevy thomasplevy merged commit b0e4c18 into dev Feb 22, 2021
@thomasplevy thomasplevy deleted the content-filter-conditions branch February 22, 2021 20:33
@thomasplevy thomasplevy changed the title content filtering conditions WIP content filtering conditions Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants