Skip to:
Content

bbPress.org

Opened 10 months ago

Last modified 4 months ago

#3389 assigned defect

First edit of a topic does not get added to revision history

Reported by: Clorith Owned by: johnjamesjacoby
Milestone: 2.7 Priority: high
Severity: normal Version: 2.0
Component: General - Content Creation Keywords: has-patch
Cc:

Description

When a moderator or higher goes to edit a topic, for whatever reason, the very first edit will not be added to the revisions.

Any subsequent edits are stored as intended, with that first edit being treated as the initial text.

This is likely due to how WordPress views a new post, where the post, and an automatic autosave are created first, and only the 3rd entry onward are treated as revisions.

Perhaps an automated autosave needs to be created alongside any new topic to resolve this (I've not tested if that's the definite cause, but it's something I notice when glancing over).

Attachments (3)

3389.patch (1.4 KB) - added by Clorith 8 months ago.
3389.2.patch (8.4 KB) - added by johnjamesjacoby 7 months ago.
3389.3.patch (1.1 KB) - added by johnjamesjacoby 6 months ago.
Like Clorith's, but using bbPress helper functions

Download all attachments as: .zip

Change History (14)

@Clorith
8 months ago

#1 @Clorith
8 months ago

  • Keywords has-patch added
  • Priority changed from normal to high

3389.patch is a workaround for the core bug that causes this unexpected behavior.

Once this is patched in core, the workaround should be removed, but it is mirroring how the customizer handles the Custom CSS feature right now and should gracefully degrade once revisions are automatically inserted.

The patch is applied to the update function, and not the insert one. This is intentional, as it will then automatically apply to all existing topics and replies, as well as future ones
, and it is done for both topics and replies.

I've also raised the priority of the ticket, as missing historical data is bad from a moderation and abuse-preventative point of view.

This ticket was mentioned in Slack in #bbpress by clorith. View the logs.


8 months ago

#3 @johnjamesjacoby
7 months ago

  • Milestone changed from Awaiting Review to 2.6.7
  • Owner set to johnjamesjacoby
  • Status changed from new to assigned
  • Version set to 2.0

#4 @johnjamesjacoby
7 months ago

Thanks @Clorith for the nudge on this one, and for the patch!

This is an interesting problem. Specifically, the way that WordPress revisions works kinda shoehorns bbPress into needing to make some decisions that WordPress really should be making for everyone.

The problem is that, at least for bbPress, WordPress revisions should not be saved after updates, but before. By saving them afterwards, there isn't a way to get ahead of the update to grab the old post content and store it. Hence, we end up with this off-by-one scenario where that first revision is always lost, either from the original topic/reply, or the first edit to it.

See the PHP Doc for wp_save_post_revision():

 * Creates a revision for the current version of a post.
 *
 * Typically used immediately after a post update, as every update is a revision,
 * and the most recent revision always matches the current post.

My revised patch takes what I learned from your ticket and patch, and moves some existing code around (with improvements) so that revisions simply always get saved. Yay!

The problem with my patch is that bbPress now doubles-up on every topic & reply in the system, saving a "first revision" of everything. That will instantly double the size of the forums, just by enabling revisions. Boo!

If WordPress switched to storing revisions before instead of after, this problem would probably go away. Or... if we decided we wanted to work around that issue and rebuilt our own revision API, that's another option, too.

TL;DR - I think bbPress needs to write its own revisions API, and then submit that feedback upstream to WordPress once that's done, because this is all a little bit backwards.

Last edited 7 months ago by johnjamesjacoby (previous) (diff)

#5 @johnjamesjacoby
7 months ago

In 7166:

Updates: clean-up & normalize topic & reply update functions.

This commit is non-functional, but rearranges some code so that _update_ functions are more predictable and easier to work with in the future.

See #3389.

#6 @Clorith
7 months ago

I guess I'm just vary of over-modifying for something that is technically a core bug, when a simple fix from the initial patch is both forward compatible for when core does fix it, and is in use by other areas of core it self so it seems like the "recommended" approach by relation.

Is there any benefit to writing a standalone revision system for bbPress that I'm missing that would make this effort worthwhile?

#7 @johnjamesjacoby
6 months ago

In 7169:

Docs: correct "arrap" to "array".

In branches/2.6 for 2.6.6. See #3389.

#8 @johnjamesjacoby
6 months ago

In 7170:

Docs: correct "arrap" to "array".

In trunk for 2.7.0. See #3389.

@johnjamesjacoby
6 months ago

Like Clorith's, but using bbPress helper functions

#9 @johnjamesjacoby
6 months ago

I think 3389.3.patch is getting there, but it still doubles the number of posts created by new topics and replies - one for the actual post, and a second for the initial revision. This is troubling for applications like forums that inevitably have more rows in the database than most blogs normally would.

I can definitely reproduce the bug, but I'm not liking with solving it this way. This fix means all bbPress forums from 2.6.7 and higher would see double the database rows, even if they're never edited later.

#10 @johnjamesjacoby
6 months ago

  • Milestone changed from 2.6.7 to 2.7

Taking this out of 2.6.x for the time being.

bbPress simply can't create duplicate rows like it would need to here.

2.6.7 can ship without this next week, and we can keep chipping away at this more.

Sorry. And thanks for understanding. 🙏

#11 @Clorith
4 months ago

Totally understandable. This is a concern we should also bring upstream to the core ticket, since the increase in entries would happen regardless when that ticket is fixed I expect?

You might be right that we need more logic here then, a function that could hook into the save action, and make a revision only if it's an existing post before the post content is replaced seems like the likely path forward here, what do you think?

Note: See TracTickets for help on using tickets.