InlineEditingDisabler::entityViewAlter() only checks if the entity is revisionable but not if it uses moderation, it should leave entities alone if they aren't using moderation.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JamesK created an issue. See original summary.

JamesK’s picture

Status: Active » Needs review
Crell’s picture

You're correct on what it's checking, but I think it's actually better as is. Even if you're not using WBM, having quick-edit on a non-latest revision (say, when browsing back through core's revision history) is a bad idea because it will result in a new revision based on that long-old revision, which is highly unlikely to be what you want. Quick Edit is for, well, "quick edits" of the live/latest revision. It's really not well suited to the concept of forward revisions at all, or even historical revisions.

See the linked issue for a lot of backstory. I'm tempted to won't-fix this issue.

JamesK’s picture

Why not leave that for #2362435: When viewing a revision, the Quick Edit, Edit, and Delete contextual link operations are available, but should not be to solve?
Basically, ModerationInformation::isLatestRevision() is pretty expensive and without this patch it get's called unnecessarily for every single entity view. I'm not using this module to fix a bug with Quick Edit, I'm using it it to apply moderation workflow to one entity type. And in my case, this entity type doesn't even use Quick Edit anyway.

Crell’s picture

Hm. OK, the performance impact is valid. Although I'm reasonably sure that can be improved just by having isLatestRevision() call getLatestRevisionId() instead. I made a separate issue for that: #2701473: isLatestRevision() can just use getLatestRevisionId(), which is cheaper

That would bring the cost down to one SQL query, rather than an extra entity load. Definitely an improvement. Is that enough of an improvement? Or are there more costs that I'm not seeing? A benchmark or profiling would be very convincing here.

balsama’s picture

Hmmm. Outside of the performance questions which were (probably) addressed in #2701473: isLatestRevision() can just use getLatestRevisionId(), which is cheaper...

> I'm not using this module to fix a bug with Quick Edit, I'm using it it to apply moderation workflow to one entity type

...is a valid point.

@crell, while I agree that quick-edit on a non-latest revision is a bad idea (without a serious UX overhaul), I also agree that fixing that problem outside of entities that WBM is touching is a little heavy handed. Unless I'm missing something, I vote for only disabling on entities under moderation.

  • Crell committed 0eb0d45 on 8.x-1.x authored by JamesK
    Issue #2700099 by JamesK: InlineEditingDisabler::entityViewAlter() doesn...
Crell’s picture

Status: Needs review » Fixed

After talking a bit further with balsama, I went ahead and committed this patch. If we find people blaming WBM in the future for quick-editing revisions and getting weird results it's trivial enough to bring back the more expansive approach.

Thanks, JamesK.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.