Posted by sun on May 24, 2009 at 2:38am
7 followers
Issue Summary
Compiled by Arancaytar in #345118: Performance: Split .module files by moving hooks:
These are the tests in question:
Advanced search form 32 1 0
Bike shed 17 1 0
Block availability 50 3 0
Block functionality 144 3 0
Field form tests 114 21 2
Trigger content (node) actions 423 9 6These are the failures I was able to replicate (35 out of 38):
Fail Other field.test 975 Entity was created
Fail Other field.test 1064 The response is an object
Fail Other field.test 1065 Response status is true
Fail Browser field.test 1034 Parsed page successfully.
Fail Browser field.test 1034 Widget 0 is displayed and has the right value
Fail Browser field.test 1035 Parsed page successfully.
Fail Browser field.test 1035 Widget 0 has the right weight
Fail Browser field.test 1034 Parsed page successfully.
Fail Browser field.test 1034 Widget 1 is displayed and has the right value
Fail Browser field.test 1035 Parsed page successfully.
Fail Browser field.test 1035 Widget 1 has the right weight
Fail Browser field.test 1034 Parsed page successfully.
Fail Browser field.test 1034 Widget 2 is displayed and has the right value
Fail Browser field.test 1035 Parsed page successfully.
Fail Browser field.test 1035 Widget 2 has the right weight
Fail Other field.test 1039 Widgets are displayed in the correct order
Fail Browser field.test 1040 Parsed page successfully.
Fail Browser field.test 1040 New widget is displayed
Fail Browser field.test 1041 Parsed page successfully.
Fail Browser field.test 1041 New widget has the right weight
Fail Browser field.test 1042 Parsed page successfully.
Fail Other search.test 286 Article node is not found with POST query and type:article.
Fail Other search.test 223 Help text is displayed when search returns no results.
Fail Other search.test 419 "Your search yielded no results" found
Fail Other search.test 423 "Your search yielded no results" found
Fail Other search.test 429 "Your search yielded no results" found
Fail Other trigger.test 45 Make sure the page has actually been created
Fail Other trigger.test 48 Make sure the publish post action fired.
Fail Other trigger.test 45 Make sure the page has actually been created
Fail Other trigger.test 45 Make sure the page has actually been created
Fail Other trigger.test 48 Make sure the make post sticky action fired.
Fail Other trigger.test 45 Make sure the page has actually been created
Fail Other trigger.test 45 Make sure the page has actually been created
Fail Other trigger.test 48 Make sure the promote post to front page action fired.
Fail Other trigger.test 45 Make sure the page has actually been createdAdditionally, I needed the following tweaks:
update_help()
module_invoke('update', 'requirements', 'runtime') resp. drupal_function_exists('update_requirements');
user_user_form()
drupal_function_exists('user_edit_form');
Comments
#1
This patch fixes all functionality and tests, except the field tests. I have no idea what is going wrong there, and it's a pain to debug and backtrace the function invocations with those countless autoloader classes, stub functions, and include files.
#2
Looks good.
But what's this doing in there?
&& user_access('administer site configuration')) {#3
function field_set_empty($field, $items) {// Filter out empty values.
$filtered = array();
- $function = $field['module'] . '_field_is_empty';
- foreach ((array) $items as $delta => $item) {
- if (!$function($item, $field)) {
- $filtered[] = $item;
+ if (module_hook($field['module'], 'field_is_empty')) {
+ $function = $field['module'] . '_field_is_empty';
+ foreach ((array) $items as $delta => $item) {
+ if (!$function($item, $field)) {
+ $filtered[] = $item;
+ }
}
}
return $filtered;
TBH, I have no idea whether this change is correct. If hook_field_is_empty() is not available, should $filtered be filled with all items, or should it be empty? The bot passes, yes, but that does not ensure that there are valid tests. ;)
else {+ drupal_function_exists('search_help');
$results = theme('search_results_listing', t('Your search yielded no results'), search_help('search#noresults', drupal_help_arg()));
}
This one we probably want to turn into an
elseif, I guess.- if (arg(0) == 'admin' && strpos($path, '#') === FALSE- && user_access('administer site configuration')) {
+ if (arg(0) == 'admin' && strpos($path, '#') === FALSE && user_access('administer site configuration')) {
That was a plain coding-style clean-up. That strange wrapping made me go nuts. ;)
That said,
arg(0)should be$arg[0]here, and the path hash check... is this actually executed in HEAD currently?#4
That wrapping was so strange I completely missed the check was there on the second line...
No idea about that # check.
#5
new patch with fix
elseiffield_set_empty()- as http://drupal.org/node/191796#191796-misc cck-modules Should implement this hook otherwise fieldAPI knows nothing about field-value and it's emptinesscheck for # is needed because of help system - to not show update_status messages on help pages
#6
Had another look through and this all looks good.
#7
As I said in #345118: Performance: Split .module files by moving hooks, I'm not convinced that we should favor a 6-8% performance gain over the developer experience. I don't think it makes sense to commit this patch until we figured out what to do with #345118.
#8
+ drupal_function_exists('user_edit_form');return user_edit_form($form_state, (isset($account->uid) ? $account->uid : FALSE), $edit);
Another example why we need something like drupal_require_function() :)
#9
Cross-post
@Dries
Even if we don't split, this patch is good. We should always check with drupal_function_exists() when executing hooks and so on.
#10
Well, but user_edit_form(), for example, is implemented in the same file that we added a drupal_function_exists('user_edit_form') too -- that sounds silly.
#11
user_edit_form()needs load only if #345118: Performance: Split .module files by moving hooks it will be removed from user.modulesuppose it unnecessary now
PS
user_edit_form()used inuser_register()too#12
Indeed. It seems that our problem right now is that we do not have a good way of automated refactoring with automatic cross-file call handling. The refactoring script can only move functions without checking references...
#13
You need to understand that everything in this patch is only required to make the .module split work. Removing changes from this patch means that functions are not found then.
#14
@sun suppose
user_edit_form()is required, I found it in 1M patch :(So actually #5 is a right one!!!
#15
But we assume that every single function lookup added in this patch will in fact be necessary after the split - and the shape of the split is still in discussion, so some functions might yet end up together even though this patch assumes they won't.
Just out of interest, I made up a simple script (in Perl, since I'm trying to learn that right now) that can check for obsolete same-file lookups. For example, after the most recent patch it will notify that update_requirements is being looked up in update.module even though it is in the same file. After the split, all these notices should be gone. :)
Edit: Also, +1 for drupal_require_function().
Having "drupal_function_exists();" on a line by itself is a) bad DX, since a developer correctly wonders why the hell we are calling a boolean function ("*_exists") and ignoring its return value, and b) bad error-handling, since on the off-chance that the function turns up missing (stale cache, missing files, typos) the check does nothing to prevent the mysterious WSOD that follows an undefined function call.
Anywhere a function is required, a better structure to use would be:
<?phpif (!drupal_function_exists('function')) {
// do one of the following:
throw new SomethingException();
drupal_watchdog('Function not found');
return;
panic();
}
?>
This structure could live very conveniently in drupal_require_function()...
#16
The last submitted patch failed testing.
#17
reroll against current HEAD
#18
This dependency each way is tricky. I think we need to postpone this until we've run more benchmarks on the other patch - then both can be RTBC very quickly after that. But needs convincing first.
#19
#20
#21
@catch: I marked this one won't fix, because I commented out the offending hook movements in the module split script/patch. I don't know whether doing this upfront makes much sense -- in the meantime, the registry.php script contains a nice list of @todos anyway.
#22
Ahh I see. I couldn't tell this won't fix from some of the others ;) Seems like duplicate is the better status but lets leave this one dead at least for now.