WordPress / twentytwentytwo

Twenty Twenty-Two, the default WordPress theme that will launch with WordPress 5.9.
406 stars 91 forks source link

Port back-compat code from twentytwentyone #306

Closed schlessera closed 2 years ago

schlessera commented 2 years ago

As a follow-up to the discussion during the core dev chat on Dec.15, this PR ports over the backwards-compatibility code from twentytwentyone. This adds admin notices and tries to avoid loading the theme on incompatible WordPress versions.

kjellr commented 2 years ago

πŸ‘‹ Hey thanks for opening the PR. Here's a link to the slack discussion for reference (signup is required to view).

I understand why this is necessary, but philosophically bundling this with the theme seems like a rather inconvenient approach β€”Β the issue of the theme failing on earlier versions is not specific to Twenty Twenty-Two. This compat file would be necessary for all block themes.

I understand that it's better than say... patching all prior versions of WP (πŸ˜…), but it still doesn't feel right to me.

cc @mtias in case you have any perspective here too.

(Also potentially related: #270)

schlessera commented 2 years ago

I understand why this is necessary, but philosophically bundling this with the theme seems like a rather inconvenient approach β€” the issue of the theme failing on earlier versions is not specific to Twenty Twenty-Two. This compat file would be necessary for all block themes.

Yes, I fully agree. Unfortunately, the extension interfaces of WordPress are ... non-existent - it just executes PHP files in certain filesystem locations. So this approach is used all over the place in both themes and plugins, where it is not WP Core that keeps things working but rather everyone else. 🀦

jffng commented 2 years ago

After some more discussion (https://wordpress.slack.com/archives/C02RP4VMP/p1641222331449800 ) and testing, I don't think we should add this to the theme.

In addition to the points raised in the discussion above, I can't recreate the scenario this is supposed to protect against. Here's what I tried:

With or without this code, I get the same result and notices.

schlessera commented 2 years ago

May I ask how you downgraded to version 5.8.2 in the above test? Not all downgrades will happen in such a way that the core updater will execute its logic or trigger any hooks.

jffng commented 2 years ago

May I ask how you downgraded to version 5.8.2 in the above test?

Downgraded via the Beta Tester plugin.

kjellr commented 2 years ago

As per the notes in https://github.com/WordPress/twentytwentytwo/pull/306#issuecomment-1016608252, I'm going to close this issue for now. If anyone would like to continue discussion, feel free to open a new issue over on Trac.

Thanks!