Over in #2232605: Themes cannot be uninstalled @catch pointed out that array_diff(array_keys($foo), array_keys($bar)); is the same as array_keys(array_diff_key($foo, $bar));. We should convert all the instances of this pattern in core

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rosinegrean’s picture

Status: Active » Needs review
FileSize
2.06 KB

The rest of the class seems ok for me.

martin107’s picture

@prics, Hello...
What you have done looks correct ....

I read the issue slightly differently

array_keys(array_diff_keys($foo, $bar))

by searching the whole repository for

'array_diff(array_keys'

I see that the bad patterns has been used 10 times throughout core.... including the 4 that your have already caught.

alexpott’s picture

Title: Clean up ConfigImporter (some more) » Use array_diff_keys() more in core
Issue summary: View changes
Status: Needs review » Needs work

I've scanned the ConfigImporter and can't find anything else obvious to clean up so how about re-scoping to fix all array_diff(array_keys(), array_keys()) in core. Seems sensible to me.

Nice patch @prics.

btw...

$supported_widget_types = array_diff(array_keys($supported_widgets), $exclude);

in EntityReferenceIntegrationTest can not be converted.

alexpott’s picture

Title: Use array_diff_keys() more in core » Use array_diff_key() more in core
Issue summary: View changes
rosinegrean’s picture

Status: Needs work » Needs review
FileSize
5.63 KB

Updated the patch.

martin107’s picture

Status: Needs review » Reviewed & tested by the community

I applied the patch and can confirm there are no more instances of the old way of doing this

Visually the patch look good to me... does exactly what is required and no more.

With the scope of the issue so specific ... I am happy to RTBC it.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: ConfigImporter-2340701-5.patch, failed testing.

rosinegrean’s picture

Status: Needs work » Needs review
FileSize
5.66 KB

Status: Needs review » Needs work

The last submitted patch, 8: ConfigImporter-2340701-8.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
5.75 KB

Simple fixup in field.api.php

hook_field_storage_config_update_forbid()

rosinegrean’s picture

Assigned: Unassigned » rosinegrean
Kingdutch’s picture

Status: Needs review » Reviewed & tested by the community

Patch applied cleanly and the following regular expression ran on the entire project yielded no results:
array_diff(\s*array_keys\(.{2,}\),\s*array_keys\(.{2,}\)\s*)

The pattern should find the following: array_diff( followed by none or any whitespace. Then array_keys( with a variable in there. This done twice separated by a comma and any possible whitespace with the closing )

Patch quality looks like that of a well done find and replace. This all together with the patch passing all existing tests makes me mark this RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: ConfigImporter-2340701-11.patch, failed testing.

Status: Needs work » Needs review
Kingdutch’s picture

I must've been blind, I thought this was all green, my apologies for marking this RTBC.

However.. the error seems to be in an unrelated part of Drupal so I re-queued the test. If I'm wrong we'll find out soon enough and we will have to do some digging.

rosinegrean’s picture

No problem, the tests passed before RTBC. I've runned in my local the tests a few min ago and they all pass.

omessaoudi’s picture

I will test this

==> Patch ran without any problem fo me too

omessaoudi’s picture

Status: Needs review » Reviewed & tested by the community
omessaoudi’s picture

Issue tags: +Amsterdam2014

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: ConfigImporter-2340701-11.patch, failed testing.

Status: Needs work » Needs review
skipyT’s picture

Status: Needs review » Reviewed & tested by the community

thanks martin107 and prics, looks ok!

webchick’s picture

Assigned: rosinegrean » catch

Sounds like catch wanted this.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed fdcf584 on 8.0.x
    Issue #2340701 by prics, martin107: Use array_diff_key() more in core.
    

Status: Fixed » Closed (fixed)

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