Posted by Dave Reid on October 9, 2008 at 5:15am
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | path.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | Needs tests |
Issue Summary
Very simple. Cleans the element-by-element array shifting done in path_admin_overview (path.admin.inc):
<?php
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):
<?php
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).
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| path-array-splice-D7.patch | 1.37 KB | Idle | Unable to apply patch path-array-splice-D7.patch | View details | Re-test |
Comments
#1
Simple patch. Do you have performance results?
#2
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.
#3
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.
#4
#5
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.
#6
#7
Ideas if this should be integrated as a new test in path.test or part of the existing path admin test?
#8
Actually here's a much better way to do this without doing any array manipulation.
#9
Looks like #8 has been committed, so this should go back to needs work per comment #5.
#10
Adding tag