Very simple. Cleans the element-by-element array shifting done in path_admin_overview (path.admin.inc):

  if ($multilanguage) {
    $header[3] = $header[2];
    $header[2] = array('data' => t('Language'), 'field' => 'language');
  }
...
    if ($multilanguage) {
      $row[4] = $row[3];
      $row[3] = $row[2];
      $row[2] = module_invoke('locale', 'language_name', $data->language);
    }

... with the wonderful PHP function array_splice, which can insert a new element in an array using array_splice($array, $offset, 0, $new_data):

  if ($multilanguage) {
    array_splice($header, 2, 0, array(array('data' => t('Language'), 'field' => 'language')));
  }
...
    if ($multilanguage) {
      array_splice($row, 2, 0, module_invoke('locale', 'language_name', $data->language));
    }

Patched and ran the path tests with 100% pass (102 passes).

CommentFileSizeAuthor
#8 318887-path-remove-array-splice-D7.patch3.52 KBDave Reid
path-array-splice-D7.patch1.37 KBDave Reid
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

Simple patch. Do you have performance results?

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

If anything, this will result in a very small increase in performance since we'd be using the native PHP function for this purpose. Main purpose of this patch was to simplify the code. I'll try and do some testing today.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Alright. Finally figured out how to get ab to test the admin/build/path page and not get 403 errors. Run on 2GHz Intel P4, 2000 users, 50 nodes (couldn't get devel_generate 7.x to use batch correctly) with path aliases and translation enabled. Before patch:

Requests per second:    4.73 [#/sec] (mean)
Time per request:       211.206 [ms] (mean)
Time per request:       211.206 [ms] (mean, across all concurrent requests)
Transfer rate:          103.22 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   203  210  16.0    207     338
Waiting:      183  189  13.4    186     296
Total:        203  210  16.0    207     338

After patch:

Requests per second:    4.77 [#/sec] (mean)
Time per request:       209.526 [ms] (mean)
Time per request:       209.526 [ms] (mean, across all concurrent requests)
Transfer rate:          104.04 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   203  209  11.2    207     307
Waiting:      169  187  10.4    186     270
Total:        203  209  11.2    207     307

Net result, very small increase in performance, but an increase nonetheless.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Reviewed & tested by the community
Dries’s picture

Dries CreditAttribution: Dries commented

Thanks for the little gem, and the performance testing. I've committed it to CVS HEAD.

Unfortunately, neither the new or the old code is tested: http://acquia.com/files/test-results/modules_path_path.admin.inc.html. As such, I'm marking this 'code needs work' so we can follow-up with some tests.

Thanks Dave.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Needs work
Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Ideas if this should be integrated as a new test in path.test or part of the existing path admin test?

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Needs work » Needs review
FileSize
318887-path-remove-array-splice-D7.patch3.52 KB

Actually here's a much better way to do this without doing any array manipulation.

AaronBauman’s picture

AaronBauman
he/him
CreditAttribution: AaronBauman commented
Status: Needs review » Needs work

Looks like #8 has been committed, so this should go back to needs work per comment #5.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Assigned: Dave Reid » Unassigned
Issue tags: +Needs tests

Adding tag

  • Dries committed 3ce6808 on 8.3.x 
    - Patch #318887 by Dave Reid: clean-up some array shifting code.

  • Dries committed 3ce6808 on 8.3.x 
    - Patch #318887 by Dave Reid: clean-up some array shifting code.

  • Dries committed 3ce6808 on 8.4.x 
    - Patch #318887 by Dave Reid: clean-up some array shifting code.

  • Dries committed 3ce6808 on 8.4.x 
    - Patch #318887 by Dave Reid: clean-up some array shifting code.

  • Dries committed 3ce6808 on 9.1.x 
    - Patch #318887 by Dave Reid: clean-up some array shifting code.