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

Not possible to override OG data when page contains global Fusion Builder container #1804

Closed
arnaudbroes opened this issue Jul 31, 2018 · 10 comments

Comments

Projects
None yet
4 participants
@arnaudbroes
Copy link
Contributor

commented Jul 31, 2018

Issue reported by Nicolas D. via e-mail.

It's not possible to override OG data (title, description) in AIOSEOP when someone is using the Fusion Builder plugin to create his pages and added a global container to his page.

To reproduce this, install both All in One SEO Pack and Fusion Builder, create a new page, add a container and save that container as a global element. Then, change the OG data and confirm that it hasn't changed after updating the page.

@wpsmort, could you please verify this again on your end?

We also have this issue with WooCommerce and Fusion Builder - #1682. We should get a fix or two out since this is a pretty popular plugin (comes with the Avada theme).

I have access to the repositories of Theme Fusion's plugins and their Avada theme. Contact me in case you need them.

@wpsmort

This comment has been minimized.

Copy link
Member

commented Aug 3, 2018

@arnaudbroes I have confirmed this by following the steps you outlined to reproduce the bug. This only happens with elements saved as Global such as Global Containers or Global Columns and it only affects the OG Title and Description.

@wpsmort wpsmort assigned contactashish13 and unassigned wpsmort Aug 3, 2018

@wpsmort wpsmort removed the Initial Review label Aug 3, 2018

@arnaudbroes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2018

Nicolas D. requested to be updated when this issue has been fixed. Just making a note to self here.

@contactashish13

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2018

@arnaudbroes have messaged you yesterday on slack for the cloudways information and the plugins. Could you please provide those?

@arnaudbroes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2018

@contactashish13 sent all steps to reproduce and the plugin file on Slack. You won't need the clone since we were able to reproduce it on a new, fresh test site.

@contactashish13

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2018

@arnaudbroes @wpsmort looks like this is because of https://github.com/semperfiwebdesign/all-in-one-seo-pack/blob/master/admin/aioseop_module_class.php#L2095-L2098.

What happens when we save a global container is this:

  1. the global container is saved as a post (say with ID 123)
  2. when we go to a post (say with ID 100) to update its social meta tags
  • the post with ID 123 is saved first
  • then the actual post with ID 100 is saved

The above code thus is fired for ID 123 first and then "locks" the method so that when it is the turn of post ID 100, the variable $update is true and the function exits.

If I comment out https://github.com/semperfiwebdesign/all-in-one-seo-pack/blob/master/admin/aioseop_module_class.php#L2097 it works as expected and the data is saved.

I'm not sure why this $update was introduced. It seems to be there from the beginning. I can't see any comments either on why this was required. @michaeltorbert any insights?

@wpsmort

This comment has been minimized.

@wpsmort wpsmort modified the milestones: 2.8, 2.9 Sep 7, 2018

@michaeltorbert

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

This was introduced in 2.0.4, though I'm not sure exactly why. The changelog just says "Bugfixes for issues reported by users in WordPress.org support forums"
https://semperfiwebdesign.com/all-in-one-seo-pack-release-history/6/

The naming of the variable $update is confusing, but this is likely meant to keep the function from running multiple times on the same update/save/publish/etc. I'm not exactly sure what the fix is, but off the top of my head maybe there's some way to see what post we're trying to save, and make sure it's currently 100 instead of 123?

@contactashish13

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2018

@michaeltorbert If the intent was to keep the function from running multiple times on the same save etc. then the approach is incorrect. The correct way is to remove the hook and then add it back again.

Do you want to (1) leave this bug as it is, (2) correct the behavior by removing/adding hooks or (3) by checking if there is a specific way this functionality can be overridden only for posts that are built with fusion builder?

@michaeltorbert

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

@contactashish13 I'm just making an educated guess as to why it's there.

Personally, I'd think we should try for (2), but do (3) only if needed.
What are your thoughts?

@contactashish13

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2018

Approach (2) works but I don't know what it breaks :)

PR: #1904

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.