PhabricatorLiskDAO: Fragment serializer cache by class
ClosedPublic
Actions

Authored by mainframe98 on Dec 24 2024, 15:31.

Details

Reviewers

aklapper
valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15064: Make Phorge compatible with PHP 8.1/8.2/8.3/8.4

Commits

rPe99e9a300367: PhabricatorLiskDAO: Fragment serializer cache by class

Summary

This restores the pre-PHP 8.1 behavior, where values of
static variables within inherited methods were independent
of each other. With PHP 8.1, this was changed to be truly
'static', which causes problems when one derivate of
PhabricatorLiskDAO defines a custom serializer but another
does not.

This came to light in T15726, but only for the Fund application,
which is a prototype, and deprecated. This fixes Fund, but
more importantly, everything else that would be broken by
this, whatever it was.

Ref: https://wiki.php.net/rfc/static_variable_inheritance

Previous stacktrace:

EXCEPTION: (RuntimeException) Undefined array key "totalAsCurrency" at [<arcanist>/src/error/PhutilErrorHandler.php:273]
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php:345]
  #1 <#2> PhabricatorLiskDAO::willWriteData(array) called at [<phorge>/src/infrastructure/storage/lisk/LiskDAO.php:1085]
  #2 <#2> LiskDAO::insertRecordIntoDatabase(string) called at [<phorge>/src/infrastructure/storage/lisk/LiskDAO.php:958]
  #3 <#2> LiskDAO::insert() called at [<phorge>/src/infrastructure/storage/lisk/LiskDAO.php:927]
  #4 <#2> LiskDAO::save() called at [<phorge>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:1405]
  [...]

Test Plan

On PHP 8.1+:

Visit http://phorge.localhost/applications/ and enable the deprecated prototype applications "Fund" and "Phortune" via "Configure"
Visit http://phorge.localhost/phortune/merchant/edit/ and create a merchant
Visit http://phorge.localhost/fund/create/ and click the "Create New Initiative" button

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

mainframe98 created this revision.Dec 24 2024, 15:31

Owners added a reviewer: O1: Blessed Committers.Dec 24 2024, 15:31

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptDec 24 2024, 15:31

mainframe98 requested review of this revision.Dec 24 2024, 15:31

Harbormaster completed remote builds in B1667: Diff 2596.Dec 24 2024, 15:31

Run unit tests

Harbormaster completed remote builds in B1668: Diff 2597.Dec 24 2024, 15:48

mainframe98 added a task: T15064: Make Phorge compatible with PHP 8.1/8.2/8.3/8.4.Dec 24 2024, 17:09

aklapper edited the summary of this revision. (Show Details)Sun, Feb 16, 16:41

aklapper edited the test plan for this revision. (Show Details)

Thanks for the patch! I can confirm that after applying the patch, on PHP 8.3 I can successful create the initiative http://phorge.localhost/I4 (and I get an unrelated PHP 8.1 deprecation warning about strlen(): Passing null to parameter #1 in src/applications/fund/xaction/FundInitiativeDescriptionTransaction.php:19)

I took the liberty to edit the commit message a bit to make reproduction easier (handy URIs to test, plus include the error message which allows easier search).

I'm accepting as O1 as it fixes the issue and as I think that I mostly understood https://wiki.php.net/rfc/static_variable_inheritance#backward_incompatible_changes but I'd kindly ask you to wait for one or two weeks for potential additional comments before landing as I do not have deep PHP knowledge. Thanks for your understanding!

This revision is now accepted and ready to land.Sun, Feb 16, 16:48

In D25859#23607, @aklapper wrote:

I took the liberty to edit the commit message a bit to make reproduction easier (handy URIs to test, plus include the error message which allows easier search).

Much obliged. I'll try and emulate this in other patches.

I'm accepting as O1 as it fixes the issue and as I think that I mostly understood https://wiki.php.net/rfc/static_variable_inheritance#backward_incompatible_changes but I'd kindly ask you to wait for one or two weeks for potential additional comments before landing as I do not have deep PHP knowledge. Thanks for your understanding!

No problem. I'll set a reminder, I can live with using a feature branch in the meantime.

aklapper mentioned this in T15726: Remove dysfunctional Fund and Phortune prototype applications.Tue, Feb 18, 07:09

Yes, I'd like to see this merged, as it fixes going to my local user profile currently only showing

PhutilAggregateException
Encountered a processing exception, then another exception when trying to build a response for the first exception.
- RuntimeException: Undefined array key "totalAsCurrency"
- RuntimeException: Undefined array key "totalAsCurrency"

on PHP 8.3.17 after performing the steps in the Test Plan above.

valerio.bozzolan added inline comments.Mon, Mar 3, 09:32

src/infrastructure/storage/lisk/PhabricatorLiskDAO.php
325–329	✅ looks good to me
331–336	Maybe this is "more like" before. See https://www.php.net/manual/en/types.comparisons.php
339–343	✅ seems good to me
345–349	Maybe this is "more like" before. See https://www.php.net/manual/en/types.comparisons.php