Skip to:
Content

bbPress.org

Opened 8 years ago

Closed 8 years ago

#1569 closed defect (fixed)

Permalinks don't work until settings page saved.

Reported by: andy Owned by: johnjamesjacoby
Milestone: 2.0 Priority: normal
Severity: blocker Version: 2.0
Component: General - Administration Keywords: needs-patch
Cc: danielbachhuber, nacin, gautam.2011@…

Description

I installed the bbpress plugin branch, activated bbpress, added a forum, and added a topic. The preview links worked but after I published I couldn't view the forum or the topic by their permalinks.

I went to the Forums settings page and clicked Save Changes. After that, the permalinks worked. Permalinks should work without having to visit and submitting the settings form. Need flush_rewrite_rules() on activation?

Attachments (4)

1569.diff (693 bytes) - added by andy 8 years ago.
This works but it's less than perfect because it doesn't regenerate the rules. It only deletes them so that the next time they are needed (e.g. to resolve a permalink) they must be regenerated. I've momentarily forgotten how to do it right.
1569.001.diff (1.9 KB) - added by cnorris23 8 years ago.
1569.2.diff (587 bytes) - added by nacin 8 years ago.
1569.003.diff (2.9 KB) - added by cnorris23 8 years ago.

Download all attachments as: .zip

Change History (19)

#1 @johnjamesjacoby
8 years ago

  • Milestone changed from Awaiting Review to 2.0
  • Owner set to johnjamesjacoby

Thanks for this. I have a flush in there but it must be firing out of order.

@andy
8 years ago

This works but it's less than perfect because it doesn't regenerate the rules. It only deletes them so that the next time they are needed (e.g. to resolve a permalink) they must be regenerated. I've momentarily forgotten how to do it right.

@cnorris23
8 years ago

#2 @cnorris23
8 years ago

  • Keywords has-patch added; plugin permalinks removed

Attached patch contains some trickery to address the issue.

#3 @johnjamesjacoby
8 years ago

So neither of these options work for me, which leads me to believe plugins need an easier way to do this in WP core. I think the logic behind activation and deactivation hooks is flawed, or I'm trying to use them incorrectly. There doesn't seem to be a way to get in between the sequences and know for sure that you're in one on the next page load.

Roll with me here.

When you deactivate bbPress, you're sending a $_GET request which reloads the page with bbPress still active. So bbPress has no idea other than by checking the $pagenow and the $_GET to know if it's time to fire the deactivation action. Even then, bbPress is still loaded and in the active_plugins option. I'm sure I'm missing something obvious.

I'm thinking of setting a transient when in some strategic places to try to counter this, but it's an obvious hack.

#4 @danielbachhuber
8 years ago

  • Cc danielbachhuber added

@nacin
8 years ago

#5 @nacin
8 years ago

bbPress hooks most rules into generate_rewrite_rules, so calling flush_rewrite_rules() on activation should be enough.

However, bbPress also has custom post types that register rewrite rules. On activation, the custom post types aren't yet registered, and thus flushing the rules will not add them to the database.

Solution is to simply register the post types then flush the rules. Attached patch should handle the issues reported here.

#6 @nacin
8 years ago

For further clarity:

bbPress hooks most rules into generate_rewrite_rules

It adds this hook on inclusion, which occurs during activation.

On activation, the custom post types aren't yet registered

... as these are hooked into init, which has already fired at the point of inclusion.

#7 @nacin
8 years ago

  • Keywords needs-patch added; has-patch removed

Just had a conversation with db and jjj about the possibility of a new hook in WP. It would be something like 'register_post_types' (and 'register_taxonomies'), which would allow plugins to register post types against it, and the hook would be fired not only on init, but on plugin activation as well.

Then I realized that we'd need a dual hook for post types and taxonomies, as sometimes you'll want to register a taxonomy before a post type -- in particular, this might need to be done for rewriting reasons. (Think slugs of books/([^/]+)/ and /books/author/([^/]+).) A hook like 'register_post_types_and_taxonomies' is not only a mouthful, but it's confusing.

So then the realization struck that the issue isn't about how post types and taxonomies are registered -- it's that activation hooks are lame.

So: Let's switch away from using an activation hook in bbPress, and instead move to an upgrade routine with a stored DB version.

That would allow us to, on admin_init, flush rewrite rules. Done. Problems solved. And it works across multisite.

For core: An option would be to begin to move away from activation hooks, implement an upgrade routine option (e.g. #WP14912), and then on activation, we could actually specify db_version = 1 automatically, which by default would trigger a rewrite rule flush. This would keep post types and taxonomies stateless while not making developers jump through hoops. Pseudo-state, if you will.

Last edited 8 years ago by nacin (previous) (diff)

#8 @johnjamesjacoby
8 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [3419]) Introduce BBP_Updater class to more accurately handle conditions where activation hooks might not fire on multisite installations, and to flush rewrite rules at the appropriate time on updates. Fixes #1569.

#9 @nacin
8 years ago

  • Cc nacin added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Let's iterate on this a bit.

@cnorris23
8 years ago

#10 @cnorris23
8 years ago

Attached patch introduces the bbp_admin_init hook, which is attached to admin_init, and then attaches bbp_setup_updater to that hook. In it's previous form, bbp_setup_updater was ineffective as it was being loaded too early. With that fixed the only hole left to plug is for situations where bbPress has been deactivated and flush_rewrite rules has been run. If bbPress is reactivated, and $bbp->db_version has been incremented upwards, everything is fine. Otherwise, bbPress permalinks will not work. Adding a transient on deactivation, then checking for it during the update() routine, could do the trick.

#11 @GautamGupta
8 years ago

  • Cc gautam.2011@… added

#12 @johnjamesjacoby
8 years ago

(In [3427]) Move 'bbp_setup_updater' action off of 'bbp_init' and late onto 'bbp_ready' to ensure alterations to the permastruct have settled. Fixes issue where a bbPress update would flush the rewrite rules too early on bbPress update. See #1569.

#13 @johnjamesjacoby
8 years ago

@cnorris23 - 'bbp_admin_init' already exists in bbp-admin.php and is run on 'admin_init' like you propose. The issue was because 'bbp_setup_updater' was triggering too early on 'bbp_init', before anything had altered the permastruct. Triggering it on 'bbp_ready' ensures that on any page load, the permalinks will get flushed accordingly. This also guarantees the db_version will be in the object cache already, which makes it a lower impact check.

Last edited 8 years ago by johnjamesjacoby (previous) (diff)

#14 @johnjamesjacoby
8 years ago

(In [3439]) Move 'bbp_setup_updater' action to 'bbp_admin_init' so update only runs when accessing wp-admin. See #1569. Props nacin.

#15 @johnjamesjacoby
8 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Closing this as fixed for 2.0. We can revisit the idea of a more bonafide updater class in 2.1 once/if we have things that need updating.

Note: See TracTickets for help on using tickets.