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());
Files: 
CommentFileSizeAuthor
#18 drupal8.menu-hook-examples.18.patch2.66 KBsun
PASSED: [[SimpleTest]]: [MySQL] 59,647 pass(es). View
#13 2108679-menu-docs-13.patch2.21 KBianthomas_uk
PASSED: [[SimpleTest]]: [MySQL] 59,441 pass(es). View
#1 2108679-menu-docs-1.patch1.74 KBianthomas_uk
PASSED: [[SimpleTest]]: [MySQL] 58,614 pass(es). View

Comments

ianthomas_uk’s picture

Status: Active » Needs review
FileSize
1.74 KB
PASSED: [[SimpleTest]]: [MySQL] 58,614 pass(es). View

And here's a patch

ianthomas_uk’s picture

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

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

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

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

ianthomas_uk’s picture

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

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

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

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

xjm’s picture

ianthomas_uk’s picture

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

Status: Needs work » Needs review
FileSize
2.21 KB
PASSED: [[SimpleTest]]: [MySQL] 59,441 pass(es). View

Patch to remove this documentation

alexpott’s picture

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

vijaycs85’s picture

Assigned: Unassigned » catch

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

sun’s picture

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

Converting to state sounds like a good idea

sun’s picture

Assigned: catch » Unassigned
FileSize
2.66 KB
PASSED: [[SimpleTest]]: [MySQL] 59,647 pass(es). View

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

Status: Needs review » Reviewed & tested by the community

Works for me.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

sun’s picture

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

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.