Subtask of #1775842: [meta] Convert all variables to state and/or config systems

The following calls and matching variable_set calls should be converted to CMI.

core/modules/menu/menu.api.php	
28   $my_menus = variable_get('my_module_menus', array());
48   $my_menus = variable_get('my_module_menus', array());
69   $my_menus = variable_get('my_module_menus', array());
CommentFileSizeAuthor
#18 drupal8.menu-hook-examples.18.patch2.66 KBsun
#13 2108679-menu-docs-13.patch2.21 KBianthomas_uk
#1 2108679-menu-docs-1.patch1.74 KBianthomas_uk

Comments

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented
Status: Active » Needs review
StatusFileSize
new 2108679-menu-docs-1.patch1.74 KB

And here's a patch

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented

Oh, if you're wondering I just removed the contents of the update hook because there wasn't really anything to update - it gets added to a list on install, removed on delete. Real code would need to be cleverer than this anyway.

Status: Needs review » Needs work

The last submitted patch, 2108679-menu-docs-1.patch, failed testing.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented
Status: Needs work » Needs review

Um, I'm pretty sure these functions never get called, so I'm going to assume that was a test suite error and try the test again.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented

OK, that was a different test that failed this time...

(For the record: Ensure the right member time is displayed.
Drupal\user\Tests\UserAdminListingTest->testUserListing())

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented

#1: 2108679-menu-docs-1.patch queued for re-testing.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented
Status: Needs review » Needs work

This shouldn't be specifying the defaults inline, and should have code to initialise the variable when the module is installed (CMI will handle the cleanup on uninstall).

catch’s picture

catch
he/him
Primary language English
commented

I know this is just an API example but this should at best be state, or possibly try to come up with another example - but using configuration for tracking state we shouldn't be recommending even in a hook example.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented

TBH the example doesn't really make much sense to me anyway.

Are example implementations required? The only code implementing this that I can find is the xmlsitemap module, which just seems to use it to set some defaults. I'm not even sure the hooks are needed in 8.x.

catch’s picture

catch
he/him
Primary language English
commented

We still have menu_menu_insert(), although that looks iffy to me.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes
Parent issue: » #1775842: [meta] Convert all variables to state and/or config systems
ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented

These hooks are rarely used, and while in D7 they were real hooks (http://drupalcode.org/project/drupal.git/blob/8649db189df672d379d2c587f3... ), in D8 they are just implementations of hook_{$entity_type}_insert (see EntityStorageControllerBase::invokeHook).

Does anyone have any objections to just removing these examples?

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented
Status: Needs work » Needs review
StatusFileSize
new 2108679-menu-docs-13.patch2.21 KB

Patch to remove this documentation

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

13: 2108679-menu-docs-13.patch queued for re-testing.

vijaycs85’s picture

vijaycs85
Location London, UK
commented
Assigned: Unassigned » catch

Seems #13 answers @catch concern at #10. Lets get @catch comment before RTBC.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Right now, we still have entity-type-specific API hook examples in .api.php files.

hook_menu_*() is comparable to hook_node_type_*(), and node.api.php contains examples for that.

Removing all of the entity-type-specific API hook examples was partially discussed in #1757586: Remove MODULE_load*() & Co functions in favor of entity_*() functions and is very debatable, so I'd recommend to not get into that discussion here.

To KISS, can we simply convert from variable to state?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Converting to state sounds like a good idea

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Assigned: catch » Unassigned
StatusFileSize
new drupal8.menu-hook-examples.18.patch2.66 KB

Or even better, ditch that entire example (because it's pointless) and use the same silly examples of hook_node_type_*().

While being there, also fixing the phpDoc and function signatures.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Reviewed & tested by the community

Works for me.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Fixed » Reviewed & tested by the community

Hm. This commit was not actually pushed? At least it doesn't appear for me?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed 
fatal: remote error: Permission denied when accessing 'drupal' as user 'webchick'

Sigh. :P pushed for real this time. Thanks.

Status: Fixed » Closed (fixed)

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