Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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) {
}
Comment | File | Size | Author |
---|---|---|---|
#27 | added-type-hinting-1903750-27.patch | 4.62 KB | benjy |
#24 | added-type-hinting-1903750-24.patch | 5.18 KB | benjy |
#20 | added-type-hinting-1903750-20.patch | 4.24 KB | benjy |
#15 | added-type-hinting-1903750-15.patch | 4.22 KB | benjy |
#13 | added-type-hinting-1903750-13.patch | 4.27 KB | benjy |
Comments
Comment #1
dawehner.
Comment #2
mitron CreditAttribution: mitron commentedIf 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.
Comment #3
benjy CreditAttribution: benjy commentedOther 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?
Comment #4
mitron CreditAttribution: mitron commentedIt 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.
Comment #5
benjy CreditAttribution: benjy commentedFrom 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
Comment #6
mitron CreditAttribution: mitron commentedIn 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.
Comment #7
benjy CreditAttribution: benjy commentedComment #8
dawehnerOn api files we use full paths, for Type-Hinting.
The rest looks really good!
Comment #9
benjy CreditAttribution: benjy commentedPatch updated to use full path in api file.
Comment #10
dawehnerThis 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.
Comment #11
benjy CreditAttribution: benjy commentedI 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.
Comment #12
mitron CreditAttribution: mitron commentedThe 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.
Comment #13
benjy CreditAttribution: benjy commented@mitron, thanks for clearing that up.
Patch attached with absolute namespaces in the api file.
Comment #14
amateescu CreditAttribution: amateescu commentedNeeds an empty line between the last use statement and the start of the doxygen block. Otherwise, this looks good.
Comment #15
benjy CreditAttribution: benjy commentedUpdated. 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.
Comment #16
benjy CreditAttribution: benjy commentedComment #17
mitron CreditAttribution: mitron commentedThere 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, theuse
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 beforeuse
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.Comment #18
benjy CreditAttribution: benjy commentedI 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.
Comment #19
mitron CreditAttribution: mitron commentedYeah, fix the user module later but fix the menu_test.module now.
Comment #20
benjy CreditAttribution: benjy commentedFixed the use statement to be in-line with what we discussed.
Comment #21
benjy CreditAttribution: benjy commentedComment #23
amateescu CreditAttribution: amateescu commentedHook 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.
hook_menu_link_presave() declares its parameter as $menu_link, so we need to update this function and change $link to $menu_link.
Comment #24
benjy CreditAttribution: benjy commentedChanges 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.
Comment #25
amateescu CreditAttribution: amateescu commentedI 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 :)
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!
Comment #26
mitron CreditAttribution: mitron commented@amateescu: Yeah we should stick to the issue, but it is good to notice other things to fix in other issues. Good catch!
Comment #27
benjy CreditAttribution: benjy commented@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. :)
Comment #28
benjy CreditAttribution: benjy commentedI've got to start remembering to do this at the same time!
Comment #29
amateescu CreditAttribution: amateescu commentedThanks, this looks great now!
Comment #30
catchCommitted/pushed to 8.x, thanks!
Comment #31.0
(not verified) CreditAttribution: commentedUpdated issue summary.