App Passwords: Regenerate .htaccess for 5.6

[TimothyBlynJacobs]

Application passwords introduced a new rule to .htaccess to handle the authorization header: ​https://github.com/WordPress/wordpress-develop/blob/5bad4e7f8d6cf67fbcea1e7c763cc41bceaa810b/src/wp-includes/class-wp-rewrite.php#L1512

We should make sure that on upgrade, the .htaccess file is regenerated.

I presume this would be done by adding an upgrade_560() function, but I'm not sure how the db version conditionals are chosen.

[SergeyBiryukov]

Just bumping $wp_db_version to the current commit number is enough to trigger the flush_rewrite_rules() call in wp-admin/admin.php​.

That's how it was done in [47018] to add the new favicon rewrite rules, no upgrade routine needed :)

flush_rewrite_rules() does a "hard" flush by default, which includes updating the .htaccess file.

Upgrade routines are generally for more complex changes, like updating the comment type from an empty string to comment in [48752] / #49236.

[TimothyBlynJacobs]

In 49534:

App Passwords: Bump database version to flush rewrite rules.

Application Passwords introduced a new Rewrite Rule to handle the Authorization header on certain systems. This bumps the database version so the change is applied to sites upon upgrading to 5.6.

Fixes #51723.

[SergeyBiryukov]

It was pointed out by @pbiron in ​#core-restapi that the .htaccess file was not updated after [49534].

While WP_Rewrite::flush_rules() gets called after a DB version bump, it only calls save_mod_rewrite_rules() if the function exists, however the function is only loaded via wp-admin/includes/admin.php exactly after the flush_rewrite_rules() call in wp-admin/admin.php​, so we do need an upgrade routine here.

For reference, it's been a while since we've added or updated .htaccess rules: [2635], [3401], [13676].

51723.diff​ works as expected in my testing.

[pbiron]

51723.2.diff​ is what I was working on as a solution: it simply does a require_once on the file where save_mod_rewrite_rules() is defined before calling flush_rewrite_rules().

This also works as expected in my testing, and is simpler :-) I can't see any downsides to it either, but maybe I'm missing something.

[SergeyBiryukov]

To summarize a bit, 51723.diff​ regenerates the .htaccess file for this specific case, and 51723.2.diff​ would do it for any DB version bump, which may or may not be necessary.

It looks like the consensus in the pre-RC1 bug scrub was to go with 51723.diff​ for now and explore 51723.2.diff​ in a future release.

[pbiron]

Replying to SergeyBiryukov:

To summarize a bit, 51723.diff​ regenerates the .htaccess file for this specific case, and 51723.2.diff​ would do it for any DB version bump, which may or may not be necessary.

It looks like the consensus in the pre-RC1 bug scrub was to go with 51723.diff​ for now and explore 51723.2.diff​ in a future release.

Yes, the consensus, with which I completely agree, is that this close to 5.6-RC1, the safest thing is to go with 51723.diff​.

Here's the rationale (when/if we get around to exploring 51723.2.diff​) I used when putting that patch together. In ​this slack thread, Sergey says:

Just bumping $wp_db_version to the current commit number is enough to trigger flush_rewrite_rules()

That's how it was done in [47018] to add the new favicon rewrite rules, no upgrade routine needed :)

But $wp_db_version is generally bumped for any rewrite rule updates like this. flush_rewrite_rules() does a "hard" flush by default, which includes updating the .htaccess file.

A few points:

1. the change in [47018] (I now see, I hadn't look at it before) involved only changes to WP rewrite rules (and didn't need a hard flush)
2. the "rewrite rule updates like this" actually do involve .htaccess changes
3. since save_mod_rewrite_rules() is not defined at the time flush_rewrite_rules() is called when db_updated is checked, the flush there has never been a "hard" flush. Based on what sergey said in that thread, I figure that was "bug"...which is fixed by that patch.

So, when/if we come back to this in a future version, maybe there's a way we can indicate (in the value for the db_updated option) whether a "hard" flush is needed and do the require_once before calling flush_rewrite_rules() only in that case.

Make sense?

[SergeyBiryukov]

Replying to pbiron:

since save_mod_rewrite_rules() is not defined at the time flush_rewrite_rules() is called when db_updated is checked, the flush there has never been a "hard" flush. Based on what sergey said in that thread, I figure that was "bug"...which is fixed by that patch.

Yeah, I guess this was never noticed because .htaccess changes are rare, the last one was 11 years ago in [13676].

So, when/if we come back to this in a future version, maybe there's a way we can indicate (in the value for the db_updated option) whether a "hard" flush is needed and do the require_once before calling flush_rewrite_rules() only in that case.

I think 51723.2.diff​ should be fine as is even without the additional indication.

This could, however, use some better documentation. Created #51805 for that :)

[SergeyBiryukov]

In 49632:

App Passwords: Regenerate the .htaccess file to add a new rule.

Application Passwords introduced a new Rewrite Rule to handle the Authorization header on certain systems.

This bumps the database version and updates the file so the change is applied to sites upon upgrading to 5.6.

Follow-up to [49534].

Props pbiron, TimothyBlynJacobs, SergeyBiryukov.
Fixes #51723.