I noticed that book_entity_info_alter() and hook_entity_info_alter() had different parameters ($entity_info vs $info), and that bothered me.
Digging through the hooks I noticed a couple more that were different (hook_menu_link_alter() vs user_menu_link_alter()).
And about half of the hook_form_FORM_ID_alter()s were missing the $form_id parameter.

Files: 
CommentFileSizeAuthor
#26 drupal-1209786-26.patch45.36 KBdman
PASSED: [[SimpleTest]]: [MySQL] 49,690 pass(es).
[ View ]
#24 drupal-1209786-24.patch47.58 KBunderq
PASSED: [[SimpleTest]]: [MySQL] 49,353 pass(es).
[ View ]
#22 drupal-1209786-22.patch47.6 KBunderq
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1209786-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#20 drupal-1209786-20.patch48.41 KBunderq
FAILED: [[SimpleTest]]: [MySQL] 49,271 pass(es), 0 fail(s), and 13 exception(s).
[ View ]
#18 drupal-1209786-18.patch48.35 KBunderq
FAILED: [[SimpleTest]]: [MySQL] 49,234 pass(es), 0 fail(s), and 124 exception(s).
[ View ]
#11 drupal-1209786-11.patch61.89 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1209786-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#9 drupal-1209786-9.patch33.33 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 33,544 pass(es).
[ View ]
#7 drupal-1209786-6.patch32.75 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 33,544 pass(es).
[ View ]
#4 drupal-1209786-everything_else-4.patch8.84 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 33,532 pass(es).
[ View ]
#4 drupal-1209786-form_alter-4.patch25.61 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 33,542 pass(es).
[ View ]
#2 drupal-1209786-everything_else-2.patch8.85 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 33,534 pass(es).
[ View ]
#2 drupal-1209786-form_alter-2.patch26.08 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 33,522 pass(es), 0 fail(s), and 8 exception(es).
[ View ]
#1 drupal-1209786-1.patch34.29 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 33,528 pass(es), 0 fail(s), and 8 exception(es).
[ View ]

Comments

tim.plunkett’s picture

Status:Active» Needs review
StatusFileSize
new34.29 KB
FAILED: [[SimpleTest]]: [MySQL] 33,528 pass(es), 0 fail(s), and 8 exception(es).
[ View ]

Attached.

tim.plunkett’s picture

StatusFileSize
new26.08 KB
FAILED: [[SimpleTest]]: [MySQL] 33,522 pass(es), 0 fail(s), and 8 exception(es).
[ View ]
new8.85 KB
PASSED: [[SimpleTest]]: [MySQL] 33,534 pass(es).
[ View ]

Okay, 34KB is pretty big, this is split into two patches, one adding $form_id to form_alters, the other with the parameter name tweaks.

Status:Needs review» Needs work

The last submitted patch, drupal-1209786-form_alter-2.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new25.61 KB
PASSED: [[SimpleTest]]: [MySQL] 33,542 pass(es).
[ View ]
new8.84 KB
PASSED: [[SimpleTest]]: [MySQL] 33,532 pass(es).
[ View ]

Fixed the failure, should work this time.

chx’s picture

Status:Needs review» Needs work

While i like unifying variable names in the first patch and giving people a reminder that form_id is readily available in the second patch, I can't agree with taking everything by reference just because we can. In fact, it's safer not to -- dont take anything by ref unless you must.

sun’s picture

+++ b/modules/update/tests/update_test.module
@@ -27,7 +27,7 @@ function update_test_menu() {
-function update_test_system_info_alter(&$info, $file) {
+function update_test_system_info_alter(&$info, $file, $type) {

not all hook implementations have to take all parameters.

typical example is hook_theme() and hook_theme_registry_alter() -- only edge-case implementations really need the additional parameters

+++ b/modules/user/user.module
@@ -1837,29 +1837,29 @@ function user_menu_site_status_alter(&$menu_site_status, $path) {
/**
  * Implements hook_menu_link_alter().
  */
-function user_menu_link_alter(&$link) {
+function user_menu_link_alter(&$item) {
...
/**
  * Implements hook_translated_menu_link_alter().
  */
-function user_translated_menu_link_alter(&$link) {
+function user_translated_menu_link_alter(&$item, $map) {

Do we have some bogus docs elsewhere?

The variable for hook_menu_link_*() should be $link, not $item, because it's a link, not a router item.

19 days to next Drupal core point release.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new32.75 KB
PASSED: [[SimpleTest]]: [MySQL] 33,544 pass(es).
[ View ]

Noted. Now it's just unifying names and adding $form_id. Nothing to break here!

tim.plunkett’s picture

Status:Needs review» Needs work

Cross-post. Will read and follow up on #6.

tim.plunkett’s picture

StatusFileSize
new33.33 KB
PASSED: [[SimpleTest]]: [MySQL] 33,544 pass(es).
[ View ]

I fixed the bits about hook_menu_link_alter, but I agree with chx over sun in terms of including arguments like $form_id.

Paraphrasing myself from IRC, "newer developers often look to core implementations AS the documentation, core should set a good example"

tim.plunkett’s picture

Assigned:Unassigned» tim.plunkett
Status:Needs work» Needs review
tim.plunkett’s picture

StatusFileSize
new61.89 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1209786-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I still have to double check the following hooks:

hook_field_formatter_info_alter(&$info) {
hook_field_formatter_prepare_view($entity_type, $entities, $field, $instances, $langcode, &$items, $displays) {
hook_field_formatter_view($entity_type, $entity, $field, $instance, $langcode, $items, $display) {
hook_field_attach_form($entity_type, $entity, &$form, &$form_state, $langcode) {
hook_field_attach_load($entity_type, $entities, $age, $options) {
hook_field_attach_validate($entity_type, $entity, &$errors) {
hook_field_attach_submit($entity_type, $entity, $form, &$form_state) {
hook_field_attach_presave($entity_type, $entity) {
hook_field_attach_insert($entity_type, $entity) {
hook_field_attach_update($entity_type, $entity) {
hook_field_attach_preprocess_alter(&$variables, $context) {
hook_field_attach_delete($entity_type, $entity) {
hook_field_attach_delete_revision($entity_type, $entity) {
hook_field_attach_purge($entity_type, $entity, $field, $instance) {
hook_field_attach_view_alter(&$output, $context) {
hook_field_attach_prepare_translation_alter(&$entity, $context) {
hook_field_language_alter(&$display_langcode, $context) {
hook_field_available_languages_alter(&$langcodes, $context) {
hook_field_attach_create_bundle($entity_type, $bundle) {
hook_field_attach_rename_bundle($entity_type, $bundle_old, $bundle_new) {
hook_field_attach_delete_bundle($entity_type, $bundle, $instances) {
hook_field_storage_info_alter(&$info) {
hook_field_storage_details($field) {
hook_field_storage_details_alter(&$details, $field) {
hook_field_storage_load($entity_type, $entities, $age, $fields, $options) {
hook_field_storage_write($entity_type, $entity, $op, $fields) {
hook_field_storage_delete($entity_type, $entity, $fields) {
hook_field_storage_delete_revision($entity_type, $entity, $fields) {
hook_field_storage_query($query) {
hook_field_storage_create_field($field) {
hook_field_storage_delete_field($field) {
hook_field_storage_delete_instance($instance) {
hook_field_storage_pre_load($entity_type, $entities, $age, &$skip_fields, $options) {
hook_field_storage_pre_insert($entity_type, $entity, &$skip_fields) {
hook_field_storage_pre_update($entity_type, $entity, &$skip_fields) {
hook_field_info_max_weight($entity_type, $bundle, $context) {
hook_field_display_alter(&$display, $context) {
hook_field_display_ENTITY_TYPE_alter(&$display, $context) {
hook_field_extra_fields_display_alter(&$displays, $context) {
hook_field_widget_properties_ENTITY_TYPE_alter(&$widget, $context) {
hook_field_create_field($field) {
hook_field_create_instance($instance) {
hook_field_update_forbid($field, $prior_field, $has_data) {
hook_field_update_field($field, $prior_field, $has_data) {
hook_field_delete_field($field) {
hook_field_update_instance($instance, $prior_instance) {
hook_field_delete_instance($instance) {
hook_field_read_field($field) {
hook_field_read_instance($instance) {
hook_field_purge_field($field) {
hook_field_purge_instance($instance) {
hook_field_storage_purge_field($field) {
hook_field_storage_purge_field_instance($instance) {
hook_field_storage_purge($entity_type, $entity, $field, $instance) {
hook_field_access($op, $field, $entity_type, $entity, $account) {
hook_field_settings_form($field, $instance, $has_data) {
hook_field_instance_settings_form($field, $instance) {
hook_field_widget_settings_form($field, $instance) {
hook_field_formatter_settings_form($field, $instance, $view_mode, $form, &$form_state) {
hook_field_formatter_settings_summary($field, $instance, $view_mode) {
hook_file_download_access($field, $entity_type, $entity) {
hook_file_download_access_alter(&$grants, $field, $entity_type, $entity) {
hook_filter_info_alter(&$info) {
hook_filter_FILTER_settings($form, &$form_state, $filter, $format, $defaults, $filters) {
hook_filter_FILTER_prepare($text, $filter, $format, $langcode, $cache, $cache_id) {
hook_filter_FILTER_process($text, $filter, $format, $langcode, $cache, $cache_id) {
hook_filter_FILTER_tips($filter, $format, $long) {
hook_filter_format_insert($format) {
hook_filter_format_update($format) {
hook_filter_format_disable($format) {
hook_help($path, $arg) {
hook_image_effect_info_alter(&$effects) {
hook_image_style_save($style) {
hook_image_style_delete($style) {
hook_image_style_flush($style) {
hook_image_styles_alter(&$styles) {
hook_language_presave($language) {
hook_language_insert($language) {
hook_language_update($language) {
hook_language_delete($language) {
hook_menu_insert($menu) {
hook_menu_update($menu) {
hook_menu_delete($menu) {
hook_node_grants($account, $op) {
hook_node_access_records(Drupal\node\Node $node) {
hook_node_access_records_alter(&$grants, Drupal\node\Node $node) {
hook_node_grants_alter(&$grants, $account, $op) {
hook_node_predelete(Drupal\node\Node $node) {
hook_node_delete(Drupal\node\Node $node) {
hook_node_revision_delete(Drupal\node\Node $node) {
hook_node_insert(Drupal\node\Node $node) {
hook_node_load($nodes, $types) {
hook_node_access($node, $op, $account, $langcode) {
hook_node_prepare(Drupal\node\Node $node) {
hook_node_search_result(Drupal\node\Node $node, $langcode) {
hook_node_presave(Drupal\node\Node $node) {
hook_node_update(Drupal\node\Node $node) {
hook_node_update_index(Drupal\node\Node $node, $langcode) {
hook_node_validate(Drupal\node\Node $node, $form, &$form_state) {
hook_node_submit(Drupal\node\Node $node, $form, &$form_state) {
hook_node_view(Drupal\node\Node $node, $view_mode, $langcode) {
hook_node_view_alter(&$build, Drupal\node\Node $node) {
hook_node_type_insert($info) {
hook_node_type_update($info) {
hook_node_type_delete($info) {
hook_file_load($files) {
hook_file_validate(Drupal\Core\File\File $file) {
hook_file_presave(Drupal\Core\File\File $file) {
hook_file_insert(Drupal\Core\File\File $file) {
hook_file_update(Drupal\Core\File\File $file) {
hook_file_copy(Drupal\Core\File\File $file, Drupal\Core\File\File $source) {
hook_file_move(Drupal\Core\File\File $file, Drupal\Core\File\File $source) {
hook_file_predelete(Drupal\Core\File\File $file) {
hook_file_delete(Drupal\Core\File\File $file) {

Status:Needs review» Needs work

The last submitted patch, drupal-1209786-11.patch, failed testing.

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned

I don't personally have time to work on this, but it's still worthwhile.

ryanissamson’s picture

@tim.plunkett I was going to help out and do this, but I am not quite sure where the discussion left off. Maybe there was something on IRC that clarified that I missed?

YesCT’s picture

Issue tags:+Novice

Here is some clarification:

+++ b/core/modules/block/block.moduleundefined
@@ -229,7 +229,7 @@ function block_block_info() {
  * Implements hook_block_configure().
  */
-function block_block_configure($delta = 0) {
+function block_block_configure($delta = '') {

the patch contains some examples. more of this needs to be done.

For all those listed functions, find the definition, check the parameter list, and then compare it to the documentation for what the parameter list should be. Make changes to make them match.

YesCT’s picture

Status:Needs work» Needs review
Issue tags:-Novice

#11: drupal-1209786-11.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Novice

The last submitted patch, drupal-1209786-11.patch, failed testing.

underq’s picture

Status:Needs work» Needs review
StatusFileSize
new48.35 KB
FAILED: [[SimpleTest]]: [MySQL] 49,234 pass(es), 0 fail(s), and 124 exception(s).
[ View ]

I reroll patch #11 to try to pass bot testing.

Status:Needs review» Needs work

The last submitted patch, drupal-1209786-18.patch, failed testing.

underq’s picture

Status:Needs work» Needs review
StatusFileSize
new48.41 KB
FAILED: [[SimpleTest]]: [MySQL] 49,271 pass(es), 0 fail(s), and 13 exception(s).
[ View ]

I replace

<?php
use \Drupal\comment\Comment;
?>

by

<?php
use Drupal\comment\Plugin\Core\Entity\Comment;
?>

Status:Needs review» Needs work

The last submitted patch, drupal-1209786-20.patch, failed testing.

underq’s picture

Status:Needs work» Needs review
StatusFileSize
new47.6 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1209786-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Reroll

Status:Needs review» Needs work

The last submitted patch, drupal-1209786-22.patch, failed testing.

underq’s picture

Status:Needs work» Needs review
StatusFileSize
new47.58 KB
PASSED: [[SimpleTest]]: [MySQL] 49,353 pass(es).
[ View ]

Retry with last commit :)

YesCT’s picture

underq, thanks for moving this forward!

dman’s picture

StatusFileSize
new45.36 KB
PASSED: [[SimpleTest]]: [MySQL] 49,690 pass(es).
[ View ]

Re-roll due to the HUGE menu refactoring that just weet in #916388: Convert menu links into entities

- multiple offset changes.
core/modules/system/system.api.php implementations of hook_menu_alter() things : removed.
core/modules/user/user.module hook_menu_alter() removed.

locale_translate_file_directory was renamed into translation_path during another patch elsewhere, so that needed to be cleaned.

So, this may be a replacement for #24
Try it out testbot...

socketwench’s picture

Issue summary:View changes
Issue tags:-Novice

Novice issue cleanup.