Variables (ie the variable_get() sort) need to be replaced with the configuration API, including a schema definition and defaults.

CommentFileSizeAuthor
#5 interdiff-2401457-2-5.txt3.3 KBkerby70
#5 admin_menu-replace-system-variables-for-config-api-2401457-5.patch8.92 KBkerby70
#3 interdiff-2401457-2-3.txt17.33 KBkerby70
#3 admin_menu-replace-system-variables-for-config-api-2401457-3.patch9.26 KBkerby70
#2 2401457.2.admin_menu.system-variables-to-config-api.patch9.73 KBjoachim
#1 2401457.admin_menu.system-variables-to-config-api.patch6.75 KBjoachim
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Active » Needs review
Parent issue: » #2398395: [meta] Port Admin Menu to Drupal 8
FileSize
2401457.admin_menu.system-variables-to-config-api.patch6.75 KB

This patch:

- adds a config schema file that defines all of admin menu's settings
- adds a schema file with the defaults, based on the values in variable_get() calls in the prior core
- replaces variable_get() calls

There is one exception: I've removed the variable_get() call for the 'admin_menu_cache_server' variable. This was undocumented, and has no UI, therefore it's just as easy and indeed preferable to set the cache to the null cache backend.

See the parent issue for prior patches this goes on top of.

Note this does not fix admin menu! After this patch, reinstall admin menu and the following code should produce a result:

  $setting = \Drupal::config('admin_menu.settings')->get('margin_top');
  debug($setting);
joachim’s picture

joachim CreditAttribution: joachim commented
FileSize
2401457.2.admin_menu.system-variables-to-config-api.patch9.73 KB

Updated patch, which also changes the 'components' settings so its sub-properties don't have the prefix 'admin_menu.'. This is because property names can't have dots in them.

kerby70’s picture

kerby70 CreditAttribution: kerby70 at Blink Reaction (now part of FFW) commented
FileSize
admin_menu-replace-system-variables-for-config-api-2401457-3.patch9.26 KB
interdiff-2401457-2-3.txt17.33 KB

Removing the settings form changes so they can be addressed in #2012194: system_settings_form() removed in drupal 8 without conflicting later. Correct one missed varable_get in admin_menu.api.php.

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs review » Needs work

Thanks for the reroll.

It's great to see other people working on this too! :D

+++ b/modules/admin_menu/config/install/admin_menu.settings.yml
--- /dev/null
+++ b/modules/admin_menu/config/schema/admin_menu.schema.yml

Unless something has changed recently, that path doesn't look right.

kerby70’s picture

kerby70 CreditAttribution: kerby70 at Blink Reaction (now part of FFW) commented
FileSize
admin_menu-replace-system-variables-for-config-api-2401457-5.patch8.92 KB
interdiff-2401457-2-5.txt3.3 KB

Thank you that's a mistake. I'll have to double check my other patches. I'm happy I have the opportunity to contribute.

Removing /module/admin_menu form file path.

kerby70’s picture

kerby70 CreditAttribution: kerby70 commented
Status: Needs work » Needs review
meaton’s picture

meaton CreditAttribution: meaton commented

I was having issues uninstalling the admin_module. I would attempt to with Drush and get the error:
Fatal error: Call to undefined function variable_del() in /srv/bindings/4d68f6588c2f4a408a149c149229b087/code/modules/admin_menu/admin_menu.install on line 22

I tried the patch above and was able to uninstall the module.
Thank you!

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs review » Reviewed & tested by the community

That sounds like an RTBC to me :)

truls1502’s picture

truls1502
he/him
Primary language Norwegian Bokmål
CreditAttribution: truls1502 as a volunteer and at SignWo AS commented
Status: Reviewed & tested by the community » Postponed (maintainer needs more info)
Issue tags: +postponed2w

I am sorry for no reply until now.

There are many issues regarding this module admin_menu which is a bit difficult for us to follow up since some of the issues might be already outdated, or is already fixed by the module or any other modules or itself core which means that the problem might no longer need to be fixed.

We can see that the issue has been created for a few years ago, I hope it is okay for you that I am postponing the issue, and give you around two weeks. If you still face the problem, could you tell us the step by step when until you get the error message or what is frustrated you, and a list of modules you are using related to admin_menu and a screenshot that might help us? So it makes us easier to reproduce your issue.

However, after two weeks with no feedback - we will close this issue. So in case, you noticed it after the issue is closed, do not hesitate to reopen it like and fill information which is mentioned above.

So before giving us a feedback, do you mind to test it again with our latest 7.x-3.x-dev?

Thank you for understanding! :)

truls1502’s picture

truls1502
he/him
Primary language Norwegian Bokmål
CreditAttribution: truls1502 as a volunteer and at SignWo AS commented
Status: Postponed (maintainer needs more info) » Closed (cannot reproduce)
Issue tags: -postponed2w

This issue has been automatically marked as closed because it has not had recent activity after the last post.

However, if you or someone is still facing the same issue as described to the issue, could you please to re-open the issue by changing the status of the issue, and add an explanation with more details which can help us to reproduce your situation.

Again, thank you for your contributions! :)