Page MenuHomePhorge
Create Task
Maniphest T15659

Transaction log entries for policy of manually created Diff lack label in Feed (which triggers a PHP 8.1 exception)
Closed, ResolvedPublic
Actions

Description

  1. PHP 8.2.12, Phorge at d4b110af260caab56936aab543041b0904e4d02a
  2. Go to http://phorge.localhost/differential/diff/create/
  3. Paste random (but valid) diff content
  4. Click Create Diff button
  5. Go to http://phorge.localhost/feed/transactions/query/advanced/ and set Object Types to Differential Diff and click Search button
  6. See two entries listed instead of one; one entry has no Transaction column entry, thus in consequence also get ERROR 8192: strncmp(): Passing null to parameter #1 ($string1) of type string is deprecated at [/var/www/html/phorge/phorge/src/applications/policy/query/PhabricatorPolicyQuery.php:314]
[2024-01-14 17:40:34] ERROR 8192: strncmp(): Passing null to parameter #1 ($string1) of type string is deprecated at [/var/www/html/phorge/phorge/src/applications/policy/query/PhabricatorPolicyQuery.php:314]
arcanist(head=master, ref.master=6c7caf3572f4), phorge(head=master, ref.master=cc964550f945)
  #0 strncmp(NULL, string, integer) called at [<phorge>/src/applications/policy/query/PhabricatorPolicyQuery.php:314]
  #1 PhabricatorPolicyQuery::isObjectPolicy(NULL) called at [<phorge>/src/applications/policy/query/PhabricatorPolicyQuery.php:318]
  #2 PhabricatorPolicyQuery::getObjectPolicy(NULL) called at [<phorge>/src/applications/policy/storage/PhabricatorPolicy.php:57]
  #3 PhabricatorPolicy::newFromPolicyAndHandle(NULL, NULL) called at [<phorge>/src/applications/transactions/storage/PhabricatorApplicationTransaction.php:458]
  #4 PhabricatorApplicationTransaction::renderPolicyName(NULL, string) called at [<phorge>/src/applications/transactions/storage/PhabricatorApplicationTransaction.php:968]
  #5 PhabricatorApplicationTransaction::getTitle() called at [<phorge>/src/applications/differential/storage/DifferentialDiffTransaction.php:43]
  #6 DifferentialDiffTransaction::getTitle() called at [<phorge>/src/applications/feed/query/PhabricatorFeedTransactionSearchEngine.php:121]
  #7 PhabricatorFeedTransactionSearchEngine::renderResultList(array, PhabricatorSavedQuery, PhabricatorHandleList) called at [<phorge>/src/applications/search/engine/PhabricatorApplicationSearchEngine.php:1078]
  #8 PhabricatorApplicationSearchEngine::renderResults(array, PhabricatorSavedQuery) called at [<phorge>/src/applications/search/controller/PhabricatorApplicationSearchController.php:275]
  #9 PhabricatorApplicationSearchController::processSearchRequest() called at [<phorge>/src/applications/search/controller/PhabricatorApplicationSearchController.php:91]
  #10 PhabricatorApplicationSearchController::processRequest() called at [<phorge>/src/aphront/AphrontController.php:29]
  #11 AphrontController::handleRequest(AphrontRequest) called at [<phorge>/src/aphront/AphrontController.php:71]
  #12 AphrontController::delegateToController(PhabricatorApplicationSearchController) called at [<phorge>/src/applications/search/engine/PhabricatorApplicationSearchEngine.php:50]
  #13 PhabricatorApplicationSearchEngine::buildResponse() called at [<phorge>/src/applications/feed/controller/PhabricatorFeedTransactionListController.php:13]
  #14 PhabricatorFeedTransactionListController::handleRequest(AphrontRequest) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:284]
  #15 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:203]
  #16 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phorge>/webroot/index.php:35]

Revisions and Commits

rP Phorge
D25517rPf81e821abf2b Mark manually created Diffs as new objects to fix empty Feed transaction entries

Related Objects

Event Timeline

aklapper created this task.Nov 3 2023, 08:51
Herald added subscribers: Cigaryno, Matthew, chris, tobiaswiese. · View Herald TranscriptNov 3 2023, 08:51
aklapper renamed this task from Duplicated Transaction log entry for Diff creation (and PHP 8.1 exception) to Transaction log entries for policy of manually created Diff lack label in Feed (which triggers a PHP 8.1 exception).Jan 14 2024, 17:35
aklapper updated the task description. (Show Details)Jan 14 2024, 17:41
aklapper added a revision: D25517: Mark manually created Diffs as new objects to fix empty Feed transaction entries.Jan 14 2024, 23:33
aklapper added a comment.EditedJan 14 2024, 23:35

My initial choice of words was wrong: It's not "duplicated". SELECT * FROM differential_difftransaction WHERE dateCreated > 1705224003; shows two transactions differing in id, phid and transactionType (differential:diff:create versus core:view-policy).

The "empty" entry is the transaction to set the initial view policy.
getTitle() in https://we.phorge.it/source/phorge/browse/master/src/applications/differential/storage/DifferentialDiffTransaction.php;4862eada5cd05236b81487b261668f2a2d72fad7$36-41 does not cover our case PhabricatorTransactions::TYPE_VIEW_POLICY so it return parent::getTitle() instead.
The parent does cover our case PhabricatorTransactions::TYPE_VIEW_POLICY in https://we.phorge.it/source/phorge/browse/master/src/applications/transactions/storage/PhabricatorApplicationTransaction.php;4862eada5cd05236b81487b261668f2a2d72fad7$943-970 but for some reason $this->getIsCreateTransaction() is not true so we wrongly end up in the else section of the code.
Manually negating the check by adding the ! to if (!$this->getIsCreateTransaction()) made the empty line in the Feed go away and correctly display '%s created this object with visibility "%s". as expected.

So getIsCreateTransaction() is wrong here. In https://we.phorge.it/source/phorge/browse/master/src/applications/transactions/storage/PhabricatorApplicationTransaction.php;4862eada5cd05236b81487b261668f2a2d72fad7$153-155 it simply calls $this->getMetadataValue('core.create', false); and returns false when the key does not exist in the metadata of the transaction.
So I assumed that we miss adding core.create to the metadata for the transaction type core:view-policy when creating a new Diff.
setIsCreateTransaction() in https://we.phorge.it/source/phorge/browse/master/src/applications/transactions/storage/PhabricatorApplicationTransaction.php;4862eada5cd05236b81487b261668f2a2d72fad7$149-151 does so, and it is called in applyTransactions() in https://we.phorge.it/source/phorge/browse/master/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php;4862eada5cd05236b81487b261668f2a2d72fad7$1315 .

Looking higher in https://we.phorge.it/source/phorge/browse/master/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php;4862eada5cd05236b81487b261668f2a2d72fad7$1175-1179 , $this->isNewObject is correctly set to true.
However, later that is not used in https://we.phorge.it/source/phorge/browse/master/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php;4862eada5cd05236b81487b261668f2a2d72fad7$1303-1317 which defines the criteria when to set $xaction->setIsCreateTransaction(true):
It checks if $xaction->getTransactionType() == PhabricatorTransactions::TYPE_CREATE.
But in our case, the transaction type is DifferentialDiffTransaction::TYPE_DIFF_CREATE instead.
Thus it is incorrectly not considered a new object and stuff fails.

The patch in D25517 does not retroactively fix this but avoids this problem for future diffs created.

valerio.bozzolan subscribed.Jan 15 2024, 09:32
valerio.bozzolan mentioned this in D25517: Mark manually created Diffs as new objects to fix empty Feed transaction entries.Jan 22 2024, 12:03
avivey closed this task as Resolved by committing rPf81e821abf2b: Mark manually created Diffs as new objects to fix empty Feed transaction entries.Feb 9 2024, 09:19
avivey claimed this task.
avivey added a commit: rPf81e821abf2b: Mark manually created Diffs as new objects to fix empty Feed transaction entries.
aklapper mentioned this in D25600: Mark created Pholio mockups as new objects to fix empty Feed transaction entries.Fri, Apr 19, 16:30
aklapper mentioned this in rPaad704d0fce5: Mark created Pholio mockups as new objects to fix empty Feed transaction entries.Thu, May 9, 09:04
Content licensed under Creative Commons Attribution-ShareAlike 4.0 (CC-BY-SA) unless otherwise noted; code licensed under Apache 2.0 or other open source licenses. · CC BY-SA 4.0 · Apache 2.0