I say this, since we are receiving Diffs from many other people without issues it seems:
- Queries
- All Stories
- Search
- Advanced Search
- Transactions
- Transaction Logs
All Stories
May 17 2023
Are you 100% sure that you are communicating with this upstream?
Just to note that, if we will be able to crash this, here is the candidate resolution:
Just a clarification: can we reproduce these issues? Can that repository be shared, or part of it, to reproduce this?
May 15 2023
TBH, I'm confused about $xaction being treated as an array in line 126.
"tooling for chains of diffs" might need its own topic, with a design of the what the whole thing should look like in an ideal world, and how to get to it.
Even if not all of the stuff fits nicely into Phorge, there's probably a lot that can work, and some conduit methods can be added to implement the rest.
Chaining Diffs is something that we would like to use more often, and it is definitely not used enough today. FreeBSD is a large codebase and making any change that is bigger than trivial often involves touching multiple parts of the system, the most difficult kind of change is architectural changes. We are limited by our tooling to a certain degree:
- Keeping everything in one Diff makes it very difficult to review as well as reach consensus, as there are often multiple responsible parties involved. It also violates the ethos of Phab/Phorge of having small atomic changes.
- Breaking changes down in smaller chunks has lots of downsides too - it is very hard to track and changes often get lost in depths of Differential.
mmm... I've never really worked where many changes are made of lots of dependent revisions - it's possible I've never even seen a chain of 3 revisions in the same repo. So I'm not sure about workflows for this kind of scenarios.
Yea, today I have about 50 lines of shell scripting that gets the different bits of data from the condon API point and then bashes them into a nearly acceptable commit message....
Ok, so here's my thoughts on moving this forward:
May 14 2023
May 13 2023
Uh, that unit test was useful indeed, and I know understand the rest of the code.
Historical commits with some comments about that regex: https://secure.phabricator.com/D7338, https://secure.phabricator.com/D7012
It might make sense to look into https://secure.phabricator.com/T13588 and https://secure.phabricator.com/D21862
Same behavior for "Mute Notifications" on http://phorge.localhost/legalpad/view/1/ with same stacktrace:
[2023-05-13 10:48:02] EXCEPTION: (PhabricatorApplicationTransactionStructureException) Attempting to apply a transaction (of class "LegalpadTransaction", with type "core:edge") which has not been constructed correctly: Transaction has type "core:edge", but that transaction type is not supported by this editor (LegalpadDocumentEditor). at [<phorge>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:1830] arcanist(head=master, ref.master=c14785c3795c), phorge(head=conduitEatMoreKittens, ref.master=2df7ea13a387, ref.conduitEatMoreKittens=2df7ea13a387) #0 <#2> PhabricatorApplicationTransactionEditor::validateEditParameters(LegalpadDocument, array) called at [<phorge>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:1209] #1 <#2> PhabricatorApplicationTransactionEditor::applyTransactions(LegalpadDocument, array) called at [<phorge>/src/applications/subscriptions/controller/PhabricatorSubscriptionsMuteController.php:58] #2 <#2> PhabricatorSubscriptionsMuteController::handleRequest(AphrontRequest) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:284] #3 phlog(PhabricatorApplicationTransactionStructureException) called at [<phorge>/src/aphront/handler/PhabricatorDefaultRequestExceptionHandler.php:41] #4 PhabricatorDefaultRequestExceptionHandler::handleRequestThrowable(AphrontRequest, PhabricatorApplicationTransactionStructureException) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:751] #5 AphrontApplicationConfiguration::handleThrowable(PhabricatorApplicationTransactionStructureException) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:296] #6 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:203] #7 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phorge>/webroot/index.php:35]
Uh right, that makes way more sense to cover numerous such cases in Arcanist instead of playing whack-a-mole in Phorge Conduit. Thanks!
Use phutil_nonempty_stringlike; update doc comment.
I think we can just have array_fuse() accept null and return an empty list, because it's only there to simplify calling sites.
May 12 2023
lol, maybe makes more sense just to remove this three lines instead of blindly replacing a function call? :P
Add minimal strict short-circuit check as proposed by Valerio
Thanks. I've patched that blindly but now I'm also able to reproduce and I also can confirm this fixes the regression.
@valerio.bozzolan I can verify that the proposed patch works for me
I was able to have that page working with:
(taken from https://we.phorge.it/T15064#8743)
Fantastic, the method is called isEmptyTextTransaction() but handles integers
When editing almanac hosts, I get this: (url /almanac/interface/edit/1/)
This is a good example of why fixing each case individually might be a bad idea, this fix was from D25202. It appears the old strlen allowed for integers too...
@valerio.bozzolan Here's a small sampling of all the errors that I encountered (I fixed each of them and got to the next one): https://we.phorge.it/P10
@valerio.bozzolan You know, I went on a spree fixing this in my local checkout. But it's super spread out all throughout the repo. The current master is broken even when I just view a revision. Then there are issues that show up in the SSH auth code when trying to push where the stack trace gets hidden (and it'll say something about ENORMOUS CHANGES, but that's a lie)
Thanks @arnold but please also share your error logs with us, otherwise we cannot fix :)
Re: all the null errors.
@aklapper Very probably, with this patch, now the log shows a resolution message with a fix to your issue