Today I edited a comment form the web interface to fix a big typo, and I noticed that the Feed was still there showing the old comment. So users were easily seeing my big typo in plain sight, in the Phorge home of Recent changes, without any indication that, that was a deprecated content, and that it was already fixed, and that a new content was available.
It is understandable that the current behavior may be a bit confusing: the old content of a comment should not be "under the spotlights" as default, and without any kind of visual help in understanding that that comment was deprecated.
To me this is not a feature, but a completion of the already-existing feature of comment editing.
Steps to reproduce:
- have phd running
- add a comment in a Task, write "sex times" - Save (and you see "sex times" in the feed)
- edit that comment, write "six times" instead - Save
What happens now:
- in the Feed (for example in your home Recent Activity) you still see the old comment content ("sex times", instead of "six times")
- and without any indication that, that comment was superseded
- and without any indication that, what the new content is
What could happen instead (proposals - feel free to add):
| Proposal name | Proposed change |
|---|---|
| "Complete proposal 1" | The Feed shows the most recent content instead AND it shows that it was "Edited" |
| "Lazy proposal 1" | The Feed just shows the most recent content instead |
| "Partial Proposal 1" | The content is omitted from the Feed - so you have to visit it. |
| "Unforgiving Blockchain 1" | The old Feed stays as-is, showing your mistake but a new Feed is created, showing the new content. Example: "Foo Edited the comment to:" |
I'm honestly inclined in trying to implement the "Complete proposal 1". If I'm not able to do it, I would be also OK with a "Lazy proposal 1". I don't think that this would be an auditing problem, since, if you want auditing, you just may like to visit the Task itself with the full edit history of that comment. I honestly also don't like the proposal called "Unforgiving Blockchain 1" since spawning a Feed for each damn comment edit is really too much folks.
Exploration
When you create a comment, phd also creates a similar entry in the database phabricator_feed.feed_storydata:
$ SELECT * FROM feed_storydata WHERE storyData LIKE '%PHID-XACT-TASK-s2szkfumltngrjk%'\G; *************************** 1. row *************************** id: 403 phid: PHID-STRY-m4ap44a4mvsiohvodcsq chronologicalKey: 7502335209367005152 storyType: PhabricatorApplicationTransactionFeedStory storyData: {"objectPHID":"PHID-TASK-cuca7wygzrk476xeo65o","transactionPHIDs":{"PHID-XACT-TASK-s2szkfumltngrjk":"PHID-XACT-TASK-s2szkfumltngrjk"}} authorPHID: PHID-USER-ktgdqflnaqzfirxfpu4z dateCreated: 1746773535 dateModified: 1746773535 1 row in set (0,001 sec)
The other tables in the same database does not contain extra data.
The only thing that is additionally happening, is a new cache entry added, about the feed content:
SELECT * FROM cache_markupcache\G;
*************************** 1. row ***************************
id: 1
cacheKey: feed:7502335209367005152:comment/378@21@KrA9lniBCyaV
cacheData: a:3:{s:6:"output";O:14:"PhutilSafeHTML":1:{s:23:" PhutilSafeHTML content";s:11:"<p>MIAO</p>";}s:7:"storage";a:0:{}s:8:"metadata";a:0:{}}
metadata: {"host":"boz-tuxedoinfinitybooksgen8"}
dateCreated: 1746774070
dateModified: 1746774070So, after you update the comment, it seems the related feed cache entry is not updated.
Workflow comparison on Comment Created VS Comment Updated:
| Topic | What | Comment Created | Comment Updated |
|---|---|---|---|
| Editor | ManiphestTransactionEditor (or specific editor) | ✅ in use | ❌ not in use |
| Editor | \ PhabricatorApplicationTransactionEditor | ✅ in use | ❌ not in use |
| Editor | \ PhabricatorApplicationTransactionEditor shouldPublish | ✅ in use | ❌ not in use |
| Cache | \ PhabricatorCacheEngine | ✅ in use | ❌ not in use |
| Editor | \ PhabricatorApplicationTransactionCommentEditor | ✅ in use (automatic) | ✅ in use (manually) |
| Controller | PhabricatorApplicationTransactionCommentEditController | ❌ | ✅ in use |
- ❓ Question "Why not App Editor": could the "Comment Updated"'s controller (PhabricatorApplicationTransactionCommentEditController) avoid to use that current editor (PhabricatorApplicationTransactionCommentEditor) that seems somehow internal, and does not trigger the story, and does not refresh the cache; and use instead the app-specific ManiphestTransactionEditor? Uhm?
- ❌ Maybe no we can get the broad editor using ManiphestTask#getApplicationTransactionEditor() that returns a ManiphestTransactionEditor().
In fact, this attempt fails miserably:
diff --git a/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentEditController.php b/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentEditController.php index 84fcecfa6f..f6a2ff7034 100644 --- a/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentEditController.php +++ b/src/applications/transactions/controller/PhabricatorApplicationTransactionCommentEditController.php @@ -58,12 +58,12 @@ final class PhabricatorApplicationTransactionCommentEditController $comment->setIsDeleted(true); } - $editor = id(new PhabricatorApplicationTransactionCommentEditor()) + $object->getApplicationTransactionEditor() ->setActor($viewer) ->setContentSource(PhabricatorContentSource::newFromRequest($request)) ->setRequest($request) ->setCancelURI($done_uri) - ->applyEdit($xaction, $comment); + ->applyTransactions($object, array($xaction)); if ($request->isAjax()) { return id(new AphrontAjaxResponse())->setContent(array());
❌ If you try the "Why not App Editor" approach, we fail miserably;
Attempting to apply a transaction (of class "ManiphestTransaction", with type "core:comment") which has not been constructed correctly: You can not apply transactions which already have IDs/PHIDs!
Workaround
Known workaround since 2025-05-09 09:00:00 CEST:
./bin/cache purge --caches remarkup
That's all. So this workaround implements the "Lazy proposal 1", so, the old entry is just refreshed to reflect the new comment.
Proposed Implementation: fix missing cache update
Given the nature of the already existing workaround, that seems related to the remarkup cache,
Given the fact that the current workaround is already in line with an already mentioned proposal (the "Lazy proposal 1"),
Given the fact that the current workaround is very probably already automatically fired when the cache expires, so causing that "Lazy Proposal 1",
Probably we should just reset that specific cache entry when an user updates a comment.