search

sybrew / the-seo-framework

The SEO Framework WordPress plugin.
https://theseoframework.com/
GNU General Public License v3.0
417 stars 47 forks source link

Better options upgrade handler #96

Closed sybrew closed 6 years ago

sybrew commented 7 years ago

The current options upgrade handler was great when it was first introduced.

...and then the options were conditionally shown, e.g.:

  1. The sitemap options are hidden when there's another sitemap plugin found.
  2. The title additions remover is hidden when your theme is doing it wrong.
  3. The object cache option is hidden when there's no external object cache handler (w3tc, wp rocket, batcache, etc.).
  4. etc.

Whenever POST is processed, all hidden options are essentially deleted from the saved options array.

The issue

The issue is that the upgrade handler will miss the options, and will notice "new" options. These options are added back to the database on every single DB update.

The user will get an annoying (although one-time) notification, and it also introduces filter incompatibilities.

Available ways to handle this

  1. Output options nevertheless, like Open Graph options; but, with a notice beneath the option.
  2. Merge default options to fill the hidden options' gaps on save.
  3. Merge missing options (that are otherwise available) with last-know set value.
  4. Ignore deleted options on upgrade handler.
  5. Set version specific upgrade handler (requires backwards compatibility list).

To summarize:

  1. Confusing, lots of useless work.
  2. Technical debt, little work.
  3. Understandable, lots of work.
  4. Technical debt, little work.
  5. Very understandable, lots of work. Requires testing and massive backwards-compatible layer for ~50 updates and all upcoming updates.

Although 5. seems to be a nice candidate, it requires new upgrade methods whenever a new option is added. This can't be automated and needs to be manually inserted every single time.

I guess I'll need to go for 3.

Bear with me, currently:

  1. Enable sitemap option.
  2. Save SEO settings.
  3. Enable sitemap plugin.
  4. Sitemap option is gone on SEO settings page.
  5. Save SEO settings again.
  6. Sitemap option is unset.
  7. Disable sitemap plugin.
  8. Sitemap option is still gone.
  9. No sitemap! And user is unaware! 🤕

This way, when you enable another sitemap plugin, and save the SEO settings, then the next time you check in the sitemap suddenly isn't disabled.

So, with 3.:

  1. Enable sitemap option.
  2. Save SEO settings.
  3. Enable sitemap plugin.
  4. Sitemap option is gone on SEO settings page.
  5. Save SEO settings again.
  6. Sitemap option is still enabled.
  7. Disable sitemap plugin.
  8. Sitemap option is back.
  9. Sitemap! 😄

Thoughts?

ramon-villain commented 7 years ago

Perhaps the approach could be semantic 😁 Instead of enabling the sitemap, it could be a checkbox for "Disable Sitemap", so the user won't have any sitemap because he decided to disable ir. And the scenario where there's another sitemap plugin, we just don't fire the SEO sitemap action. The checkbox for disabling would stay unchecked.

It's like 3, but more understandable, at least for me.

sybrew commented 7 years ago

@ramon-villain, thanks for the feedback!

Although it's a nice idea, in the end that would make it actually even more work (and confusing for most users and me).

Food for thought: Mixing "negative" and "positive" (enable/disable) options is a design disaster and I have a very difficult time implementing the right wording. This is because disabling mostly implies removing something negative caused by external conditions. Whereas enabling something would imply a positive addition to your website.

We could argue that the "enable no-index/archive/follow" options should actually be "disable indexing/archiving/following". Now, those options add an extra tag/condition, so there's the counter argument... ugh 😅

But I digress... because this isn't the actual issue.

Back to the issue: The issue is that any option could be conditional in any situation. Be it now or in the future. Some might even wish to remove some metaboxes from view for their users.

Converting all those options each time from positive to negative requires another upgrade handler checking for them (an example: The Twitter Card upgrader).

There are more arguments supporting this, but I think the point (at least from a coding perspective) is clear enough for now 😄

What I want to achieve: A solution that will work now and in the future. Without ever needing to look back; thus completely automated.

ramon-villain commented 7 years ago

Yes, you're right, we can't work for an unique option, it should respect the whole scenario/experience. Therefore I guess you are in the right road.

sybrew commented 6 years ago

The proposed resolution will lead to bugs if not tightly controlled. Moved to a later version where we output options through a schematic format, rather than plain HTML.

We're maintaining resolution 5 in the meantime, without backward compatibility until it waters down. If we can assure almost all sites are running TSF 3.0 or later, this shouldn't be an issue anymore.

sybrew commented 6 years ago

The upgrade handler and all affected settings are now handled differently since the commits listed above. These changes make this issue obsolete. We'll see what needs to be done differently later.