Posted by dawehner on January 30, 2013 at 11:52pm
7 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | menu_link.module |
| Category: | task |
| Priority: | normal |
| Assigned: | benjy |
| Status: | closed (fixed) |
| Issue tags: | Novice |
Issue Summary
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
<?php
function user_menu_link_presave($link) {
}
?>should be
<?php
function user_menu_link_presave(MenuLink $link) {
}
?>
Comments
#1
.
#2
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.
#3
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?
#4
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.
#5
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
#6
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.
#7
#8
+++ 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!
#9
Patch updated to use full path in api file.
#10
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.
#11
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.
#12
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
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.
#13
@mitron, thanks for clearing that up.
Patch attached with absolute namespaces in the api file.
#14
+++ 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.
#15
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.
#16
#17
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
usestatements shows that in 19 out of 20 cases, theusestatements 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 beforeusestatements.We should clean up user.module elsewhere, perhaps in a new issue. However, the menu_test.module
usestatement was added in this patch, so perhaps that should be moved to after the @file comments.#18
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.
#19
Yeah, fix the user module later but fix the menu_test.module now.
#20
Fixed the use statement to be in-line with what we discussed.
#21
#22
The last submitted patch, added-type-hinting-1903750-20.patch, failed testing.
#23
+++ 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.
#24
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.
#25
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!
#26
@amateescu: Yeah we should stick to the issue, but it is good to notice other things to fix in other issues. Good catch!
#27
@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. :)
#28
I've got to start remembering to do this at the same time!
#29
Thanks, this looks great now!
#30
Committed/pushed to 8.x, thanks!
#31
Automatically closed -- issue fixed for 2 weeks with no activity.