Problem/Motivation

This is a follow up on a previous iteration: #3164210: Refactor array_merge() usage in loops as possible for performance.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

None

Data model changes

None

Release notes snippet

CommentFileSizeAuthor
#30 phpstorm.png119.68 KBmxr576
#20 interdiff_1900-3385.txt0 bytesmxr576
#20 interdiff_1892-3385.txt2.48 KBmxr576
#20 interdiff_1892-1900.txt2.48 KBmxr576

Issue fork drupal-3266568

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mxr576 created an issue. See original summary.

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
Issue tags: +Performance

mxr576 opened merge request !1892

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
Status: Active » Needs review
mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented

Also addressed the issue was described in #3044373 but was incorrectly fixed.

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented

mxr576 opened merge request !1900

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Status: Needs review » Needs work
mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
Status: Needs work » Needs review

@yogeshmpawar why needs work? The 9.4.x branch is passing, I assume the 10.x upstream branch is broken somehow, I keep rebasing these MRs, but MR 1892 is ready for review.

quietone made their first commit to this issue’s fork.

quietone’s picture

quietone commented

This will be committed to Drupal 10 first, so a MR for D10 is needed before this is ready for review.

Tests were failing on 10 because of Deprecate autovivification on false. Not sure this is the best way to fix it but I thought I would have a go at it.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

MR 1900 appears to have failures.
Also will have to be updated for 10.1 now please.

sourabhjain made their first commit to this issue’s fork.

sourabhjain opened merge request !3385

sourabhjain’s picture

sourabhjain
Primary language English
commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Still failures

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented 
08:26:01 
08:26:01    ✖ 14) Olivero/oliveroSearchFormTest
08:26:01 
08:26:01    – search wide form is accessible and altered (5.62s)
08:26:01 
08:26:01    → ✖ NightwatchAssertError
08:26:01    Timed out while waiting for element <#edit-keys--2> to be present for 5000 milliseconds. - expected "visible" but got: "not found" (5095ms)
08:26:01 
08:26:01     Error location:
08:26:01     /var/www/html/core/tests/Drupal/Nightwatch/Tests/Olivero/oliveroSearchFormTest.js:
08:26:01     –––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––––
08:26:01      34 |       .assert.attributeEquals(searchButtonSelector, 'aria-expanded', 'false')
08:26:01      35 |       .click(searchButtonSelector)
08:26:01      36 |       .waitForElementVisible(searchWideInputSelector) 
08:26:01      37 |       .assert.attributeEquals(searchButtonSelector, 'aria-expanded', 'true')

Are these failures are actually related to this change?

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
StatusFileSize
new interdiff_1892-1900.txt2.48 KB
new interdiff_1892-3385.txt2.48 KB
new interdiff_1900-3385.txt0 bytes

Btw, interdiffs between open MR-s.

1900 and 3385 are basically the same.

mxr576 closed merge request !1900

mxr576 closed merge request !1892

mxr576 reopened merge request !1892

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented

Now 1892 and 3385 are in sync and target branch changed. 3385 can be closed as duplicate.

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented

hm, when I rolled back @quietone's two changes then these errors have disappeared in my local. Let's see what happens if I commit that change...

Commit ids:
d66f152f88d1590573bd4cc9fa855a1f0f935461
8064cac900af691ce05e3ae0b04527717233845e

09:32:10 Drupal\Tests\content_moderation\Kernel\ContentModerationPerm   0 passes   1 fails                            
09:32:10 Drupal\Tests\content_moderation\Kernel\ContentModerationStat   0 passes   1 fails
mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented

So d66f152f88d1590573bd4cc9fa855a1f0f935461 is needed for sure.

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
Status: Needs work » Needs review

MR1892 is ready for review \o/

smustgrave’s picture

smustgrave commented
Issue tags: +Needs Review Queue Initiative

Looking at MR 1892

Do we care about other instances? How do you best search for these?

The changes being proposed look good so +1 from me if point above is none issue.

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented

Do we care about other instances?

We can, but I'd probably do this iteratively since this MR already contains several changes.

How do you best search for these?

I'd use a regex like this and audit the surrounding code whether it is a loop or not and what is happening there: array_merge\([^.]+

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
Issue summary: View changes
StatusFileSize
new phpstorm.png119.68 KB

Do we care about other instances?

We can, but I'd probably do this iteratively since this MR already contains several changes.

Okay so I have added a few more fixes... hopefully they do not cause more work on this MR...

BTW, PHPStorm with Php Inspections ​(EA Extended)​ plugin enabled nicely highlights these problems.
PHPStorm

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Will see what the committers say. Fine with doing it in waves but may need a follow up for tracking that.

  • catch committed 8df6544e on 10.1.x 
    Issue #3266568 by mxr576, sourabhjain, quietone: Refactor array_merge()...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

For scope I think I might have swapped out the array_merge([], ...$foo) when it's already outside a loop to its own issue, so that only the loops need reviewing. But by the time I'd thought of this, I'd already reviewed the patch which looks good. I think it's OK if we don't get all of these, the only way to guarantee not introducing any more would be a phpstan rule (if that's even possible).

Committed 8df6544 and pushed to 10.1.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.