Not before time!

Had this issue occur elsewhere, so put a general 'fix' for it:

To find it, I needed to put in loads of debugging. It seems to be an issue with PHP deferring the execution of the object destructor until after the Imbalanced AphrontWriteGuard check, even though the object was unset prior to the check.

I wonder if xhpast can be ditched. Eg:
For linters, use something off-the-shelf such as phpcs (https://github.com/squizlabs/PHP_CodeSniffer)
For diviner parsing of PHP code, I look at the class documentation (eg https://we.phorge.it/book/libphutil/class/ArcanistConduitCallFuture/) and question as to whether this provides anyone with any benefit.

In T15047#1308, @speck wrote:

Sorry, I'm not explaining it very well. For the database engine to use index (col1, col2, col3), col1 must be specified somewhere in the WHERE clause, but it's precise location is irrelevant - the database engine will work it out. As to why we need both indexes, there's a good explanation on https://use-the-index-luke.com/sql/where-clause/the-equals-operator/concatenated-keys

@avivey - the order of the keys is irrelevant when it comes to determining uniqueness, but extremely important in other respects. In order for an index to be used when selecting rows, the database must be provided with values for the index fields, in the order specified. Ie you always need to specify a value for the first field, and can get away without successive ones.

@valerio.bozzolan agreed - I'm not proposing to remove the (dst, type, src) index, just the uniqueness constraint on that index. Sorry for not making that clear.

Possible further enhancements, which I am loathe to do when trying to fix an existing bug, so perhaps as a future diff:

As per @avivey, update ArcanistPhpunitTestResultParser.php to call ArcanistXUnitTestResultParser and remove it's own XUnit parsing code.

@speck , @valerio.bozzolan
Sorry, yes, it's updating the test engine to parse phpunit's junit output, rather than the json output which no longer exists.

Testing, using https://github.com/campbsb/example-phorge-php-project.git and breaking it so as to cause a test failure, we get:

• Add LIBXML_NONET option
• Remove $last_test_finished and the !last_test_finished block, as that is always true

When a phriction markup page is generated , its synchronous, when a diffusion markup page is generated it's asynchronous.
This is going to take a bit of thought.

Can no longer replicate the fault.

I can no longer replicate the fault, so am going to scrap this one.

One of my favourite updates in my workplace was to store counts of feature usage per month in a table. Move forward 3 years, and whenever we need to update some code in a feature, we check for it in the table, and if it's not been used in 3 years we just scrap the feature.

Skip lint (javelinsymbols)

Great suggestions

• Implemented celerity_generate_unique_node_id()
• Implemented .collapsible-content > ul.remarkup-list CSS

Loved the comment, as otherwise I wouldn't have understood what was going on in the CSS. I've reworded it - hope that's OK.

I am quite happy with this now. Spent way too long on it, so I hope you like it too!

• Refactored the PHP code to build the collapsible button & div into buildCollapsibleIndex()
• Define the javelin call config first, then refer to it - saves defining the variables independently!
• Create an icon field in the button, allowing for open and close icons to be set
• Make both the icon and the text optional in the collapsible javascript - if you just want an icon, fine. Just text, also fine. Neither - sure!

The problem there is that by requiring collapsiblejx to have knowledge of the 'collapsible-toggler-label', we introduce tight coupling to it's caller. (see how adding 'jx' on to the end of collapsiblejx has made it uniquely identifiable?)
A better solution would be to have the collapsiblejx config contain 'btn_open_icon' and 'btn_close_icon' icons. I'll have a go...

• move duplicated button text into a variable
• improve formatting

• Renamed button/div IDs to remove spaces
• Renamed diffusion-collapsible-container to collapsiblejx
• Added margin-top: 10px; in the collapsible content - this overrides the higher up margin-top: 0px
• Moved webroot/rsrc/js/application/diffusion/collapsible-container.js to webroot/rsrc/js/core/behavior-collapsiblejx.js
• Renamed the JX from diffusion-collapsible-container to collapsiblejx - the single word collapsible was a bit too common if you 'git grep' for it.

Renamed butt to btn as per PR. Much more common abbrieviation!

Happy to take your suggestions with a couple of changes

• Remove index from the config labels - we've made collapsible-container.js generic, and not just for an index.
• The reason for the if/else block was to enable long variable/attribute names so as to have self-documenting code. I've done away with it, but we do need a comment instead so as to show why we associate the 'push this to close' text with the open state and vice-versa.

As an aside, what do you think of https://github.com/campbsb/example-phorge-php-project - should we put a selection of example projects on https://we.phorge.it, either as individual repos, or one repo containing a bunch of examples, or somewhere in the arcanist repo ?

More complete test case added to D25472 which demonstrates the following error:

I guess the documentation needs an update on page https://we.phorge.it/book/phorge/article/arcanist_lint_unit/ and/or https://we.phorge.it/book/phorge/article/arcanist_new_project/

6 year old in-your-face unreported bugs like these make me wonder how many people use Phabricator/Phorge :-(

Any problems with this fix?

Is switching to using phutil_nonempty_scalar a blocker?
I don't think it should be because otherwise, we could have just done a bulk change across the whole codebase converting

• Javascript updated to make collapsible containers more generic, and not specific to indices
• Removed unused 'active' as per review
• Moved setting the display of the container from Javascript to CSS as per review
• Added i18n support for the button text (no need for a TODO!)
• Extended variable/attribute names to make the code a bit more self-documenting

Remove "'hidden' => true," line from javelin config - no longer required.

Update the button such that it toggles between 'Open Index' and 'Close Index'

In D25457#13055, @valerio.bozzolan wrote:

Non-blocking stuff:

3. Maybe we should also evaluate this?
   

Rebuild celerity map

Skipping lint because I cannot get javelinsymbols from https://github.com/phacility/javelin/tree/master to compile under OSX
Actually, it's the out-of-support 13 year old libfbjs (from https://github.com/facebookarchive/libfbjs) from it which won't compile.
I recommend javelin linting is dropped, and replaced with a more modern javascript linter.

Move javascript into a Javelin event.
Correct mistake in Javelin documentation.

In D25457#13043, @valerio.bozzolan wrote:

I like the general idea but I don't know how to improve it graphically. I mean, this is the current rendering for a simple README of two sections:

I don't think any document with a header to text line ratio of 2:3 is going to look good without customised formatting.

The Javascript is inline for now, and we can look at putting it somewhere better if this diff is worth pursuing. Suggestions of where to put it welcome!

Replace method_exists with instance of, as per review

Replacing 'new PhutilSafeHTML' with 'phutil_implode_html' as per review. Much nicer!

Looks OK from XSS perspective - protection already in place. Ready for review!

Need to put in some XSS protection.

Restore fixed interpreter-test.txt

An example of the TOC in action is in the diviner pages.

smallcow test case added.

Replace $m with $matches, as per review.

This issue has been a pain for years. Never investigated it before...

Fix unit test

Lint & fix xml unit test