Follow-up of #916388-161: Convert menu links into entities

All the different kind of hooks like hook_menu_link_presave should use type hinting, so

function user_menu_link_presave($link) {
}

should be

function user_menu_link_presave(MenuLink $link) {
}
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Issue tags: +Novice

.

mitron’s picture

If the purpose of the issue is to add typing hinting, then the code and comments should already be in place for this. However, there does not appear to be any code that is ready except for type hinting.

benjy’s picture

Other than a couple in the user module they all have comments and param comments.

I can go through and add in all the type hinting if you want?

mitron’s picture

It appears that this issue is for a task that can't be completed until the patch from #916388: Convert menu links into entities is committed.

benjy’s picture

From first look $link was of type MenuLink on 8.x HEAD. I think they've maybe been converted but are waiting on something else?

http://drupal.org/node/1914008

mitron’s picture

In the file menu_link.api.php, the comments for the below functions now state that the parameter is a MenuLink.

hook_menu_link_presave($menu_link)
hook_menu_link_insert($menu_link)
hook_menu_link_update($menu_link)
hook_menu_link_delete($menu_link)

So it looks like you can update the type hinting.

benjy’s picture

Assigned: Unassigned » benjy
Status: Active » Needs review
FileSize
4.26 KB
dawehner’s picture

+++ b/core/modules/menu_link/menu_link.api.phpundefined
@@ -1,5 +1,6 @@
+use Drupal\menu_link\Plugin\Core\Entity\MenuLink;

@@ -47,7 +48,7 @@ function hook_menu_link_load($menu_links) {
+function hook_menu_link_presave(MenuLink $menu_link) {

On api files we use full paths, for Type-Hinting.

The rest looks really good!

benjy’s picture

Patch updated to use full path in api file.

dawehner’s picture

This looks great, though i'm wondering whether we should better use absolute namespaces, so "\Drupal\user\Plugin..." instead of "Drupal\user\Plugin"
just as example.

benjy’s picture

I did think that myself. I looked at a few examples in the node module and that had a relative namespace. I've just looked again and there are also absolute namespaces in the node and block modules.

I personally think absolute is probably more accurate but either way let's make a decision and make them all consistant across the board.

mitron’s picture

The standard in doc (like following @param) is to use fully qualified class names with the leading backslash. All these are correct in the patch. See Drupal API documentation standards for classes and namespaces.

The standard for use statements comes from PHP and if there is already a backslash in the used namespaces, the standard is to not include the leading backslash. All use statements are correct in the patch. See http://php.net/manual/en/language.namespaces.importing.php. From about the middle of the page

Note that for namespaced names (fully qualified namespace names containing namespace separator, such as Foo\Bar as opposed to global names that do not, such as FooBar), the leading backslash is unnecessary and not recommended, as import names must be fully qualified, and are not processed relative to the current namespace.

If we are not going the import the class with a use statement, even if we are in the global namespace where it is technically not necessary, let's be consistent with the @param documentation and so instead of
function hook_menu_link_insert(Drupal\menu_link\Plugin\Core\Entity\MenuLink $menu_link)
Let's insert the leading backslash
function hook_menu_link_insert(\Drupal\menu_link\Plugin\Core\Entity\MenuLink $menu_link).

If we see the same thing in the @param as in the function statement, it might be just a tiny but easier and faster to understand. If we leave out the leading \, you have to momentarily ask yourself, "We are in the global namespace, right?" If you put that leading backslash in there, you don't have to even think about it.

benjy’s picture

@mitron, thanks for clearing that up.

Patch attached with absolute namespaces in the api file.

amateescu’s picture

Component: menu system » menu_link.module
Status: Needs review » Needs work
+++ b/core/modules/user/user.module
@@ -9,7 +9,7 @@
+use Drupal\menu_link\Plugin\Core\Entity\MenuLink;
 /**
  * @file

Needs an empty line between the last use statement and the start of the doxygen block. Otherwise, this looks good.

benjy’s picture

Updated. I also noticed since you pointed this out that the user module seems to incorrectly have all of it's use statements above the @file block.

benjy’s picture

Status: Needs work » Needs review
mitron’s picture

Status: Needs review » Needs work

There does not appear to be an official standards statement that the @file comment block comes before any other statements.

Examining the first 20 files (alphabetically) in core that contain use statements shows that in 19 out of 20 cases, the use statements comes after the @file comments. In one case there were not any @file comments. So it is reasonable to assert that there is a consensus among core developers that @file comments come before use statements.

We should clean up user.module elsewhere, perhaps in a new issue. However, the menu_test.module use statement was added in this patch, so perhaps that should be moved to after the @file comments.

benjy’s picture

I don't like the idea of a use statement above and below the file doc?

Let's leave it where it is for now and create a new issue to fix the user module once this has been committed.

mitron’s picture

Yeah, fix the user module later but fix the menu_test.module now.

benjy’s picture

Fixed the use statement to be in-line with what we discussed.

benjy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, added-type-hinting-1903750-20.patch, failed testing.

amateescu’s picture

+++ b/core/modules/system/tests/modules/menu_test/menu_test.module
@@ -591,30 +593,39 @@ function menu_test_menu_name($new_name = '') {
 /**
  * Implements hook_menu_link_insert().
  *
+ * @param \Drupal\menu_link\Plugin\Core\Entity\MenuLink $menu_link
+ *   A menu link entity.
+ *
  * @return
  *  A random string.
  */
-function menu_test_menu_link_insert($item) {
+function menu_test_menu_link_insert(MenuLink $item) {

Hook implementation don't need to declare their parameters or return values, so @param (that's added by this patch) and @return should be removed from all implementations.

+++ b/core/modules/user/user.module
@@ -1127,7 +1128,7 @@ function user_menu_site_status_alter(&$menu_site_status, $path) {
-function user_menu_link_presave($link) {
+function user_menu_link_presave(MenuLink $link) {

hook_menu_link_presave() declares its parameter as $menu_link, so we need to update this function and change $link to $menu_link.

benjy’s picture

Status: Needs work » Needs review
FileSize
5.18 KB

Changes made, I also removed @param and @return from any other hook implementations in the menu_test.module file.

New patch attached. Not entirely sure why the last one failed.

amateescu’s picture

Status: Needs review » Needs work

I also removed @param and @return from any other hook implementations in the menu_test.module file.

I also tend to get carried away like that and fix more stuff than necessary in one patch, but the general rule is that patches should only affect the scope of the issue, so please undo those two @return removals and leave them for another cleanup patch :)

+++ b/core/modules/user/user.module
@@ -1127,19 +1128,19 @@ function user_menu_site_status_alter(&$menu_site_status, $path) {
+      $menu_link->plid = 0;

Extra indent added here (4 spaces instead of 2), needs to be reverted.

After this, we should be done here. And thanks for the quick turnaround times!

mitron’s picture

@amateescu: Yeah we should stick to the issue, but it is good to notice other things to fix in other issues. Good catch!

benjy’s picture

@amateescu, I've removed the param added by my patch and fixed the spacing.

I hope every time an issue gets resolved on drupal.org we don't find 2 or 3 more otherwise the issue queue will be growing almost exponentially. :)

benjy’s picture

Status: Needs work » Needs review

I've got to start remembering to do this at the same time!

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, this looks great now!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.