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

Fix conflict by disabling Yoast initialization on Divi preview #6342

Merged
merged 4 commits into from
Jan 6, 2023

Conversation

renatho
Copy link
Contributor

@renatho renatho commented Dec 28, 2022

Fixes temporarily #6112

Changes proposed in this Pull Request

  • In order to fix the conflict between Learning Mode + Divi + Yoast, this PR disables the Yoast initialization when it's running the Divi preview in the editor.
  • It's important to mention that I didn't find the root cause of the issue yet, but since this solution is probably safe I decided to stop investing much more time in that. I don't think a user would want to optimize the SEO in the Divi preview in the editor - unless there's any other implication that I didn't realize.

My findings from the investigation

  • \add_action( 'wpseo_head', [ $this, 'present_head' ], -9999 ); in Yoast will make it call the get_the_excerpt (Post_Helper::get_the_excerpt). If we don't call the get_the_excerpt there, the problem doesn't happen.
  • There's also a method in Divi that I suspect a little, but I couldn't figure out too much from that. The method is ET_Builder_Element::reset_element_indexes. It seems it tries to fix something related to the_content recursions. And my suspicious is that it has something related to the problem because with the Learning Mode we make some nested the_content calls in Sensei\Blocks\Course_Theme\Course_Content::render_content.

Testing instructions

  • See the issue details, and make sure you can reproduce it using the trunk.
  • Check that running the code from this branch the problem doesn't happen anymore.

@renatho renatho requested a review from a team December 28, 2022 21:01
@renatho renatho self-assigned this Dec 28, 2022
* It avoids an existing conflict that prevents the preview from loading.
*/
function sensei_fix_divi_yoast_conflict() {
if ( isset( $_GET['et_block_layout_preview'] ) && '1' === $_GET['et_block_layout_preview'] ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ideal would be to also have a check to run it only in lessons with Learning Mode, but it's too early to check this.

@codecov
Copy link

codecov bot commented Dec 28, 2022

Codecov Report

Merging #6342 (86d4422) into trunk (f55d8f2) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              trunk    #6342   +/-   ##
=========================================
  Coverage     45.97%   45.97%           
  Complexity     9546     9546           
=========================================
  Files           459      459           
  Lines         33831    33829    -2     
  Branches        275      275           
=========================================
  Hits          15553    15553           
+ Misses        18073    18071    -2     
  Partials        205      205           
Impacted Files Coverage Δ
includes/class-sensei-posttypes.php 11.18% <0.00%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52e6a6d...86d4422. Read the comment docs.

@renatho renatho changed the title Disable Yoast initialization on Divi preview Fix conflict disabling Yoast initialization on Divi preview Jan 5, 2023
@renatho renatho changed the title Fix conflict disabling Yoast initialization on Divi preview Fix conflict by disabling Yoast initialization on Divi preview Jan 5, 2023
jom
jom previously approved these changes Jan 5, 2023
Copy link
Member

@jom jom left a comment

Choose a reason for hiding this comment

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

This fixes the described issue. I wonder if after this is merged we should transfer your notes to a new issue. It makes sense to me to merge this now, but we should probably prioritize finding a less blunt solution.

@jom jom self-requested a review January 5, 2023 16:02
Copy link
Member

@jom jom left a comment

Choose a reason for hiding this comment

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

Actually, I just had a thought. This allows non-authenticated users to prevent loading Yoast from any request. It seems a bit too powerful. Is there a way we can restrict it just to the required request?

@jom jom self-requested a review January 5, 2023 16:05
@renatho
Copy link
Contributor Author

renatho commented Jan 5, 2023

Actually, I just had a thought. This allows non-authenticated users to prevent loading Yoast from any request. It seems a bit too powerful. Is there a way we can restrict it just to the required request?

Hi Jake! Thank you for your review! That's a great point! Since it's in the plugins_loaded hook, we don't have too much information to check, but I think the extra check I added is enough: eea3a40. It will check if the request is coming from the admin. WDYT?

we should probably prioritize finding a less blunt solution.

Totally agree! For now, I just added this comment to the original issue, changing it to low priority. So we can keep all the history of this issue in the same issue. Does it make sense?

@renatho
Copy link
Contributor Author

renatho commented Jan 5, 2023

@jom, I checked that we could check the user roles (edit_lessons, for example) in the plugins_loaded, but based on your idea, I sent another commit here changing the hook and removing all actions from wpseo_head: 932fc83. So we check if they can edit specifically that post.

Update: I also added another check to remove the action only when it's a lesson with learning mode enabled. 86d4422

Copy link
Member

@jom jom left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for picking this one up. 🪂

@renatho renatho merged commit e49bc41 into trunk Jan 6, 2023
@renatho renatho deleted the fix/divi-yoast-conflict branch January 6, 2023 13:06
@donnapep donnapep added this to the 4.10.0 milestone May 1, 2023
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.

3 participants