change all instances of variable_set/get/delete menu_masks to the state system.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ACF’s picture

Menu masks updated.

ACF’s picture

Status: Active » Needs review
alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. No upgrade path needed since the menu router will be rebuilt during d7 to d8 upgrade.

Tested - thanks for the work.

chx’s picture

This one is good, yes, thanks.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Actually just realised that we should be cleaning up the variables table on upgrade! :)

So in this case we need a menu_update_N function to do a update_variable_del('menu_masks')

Lars Toomre’s picture

Maybe it can go into the same menu_update_N function that also includes update_variable_del('menu_rebuild_needed')?

alexpott’s picture

@Lars there will probably a clean up of the update functions but doing that now would be premature - adding a new function is the way to go at the moment.

Lars Toomre’s picture

Fair enough @alexpott! I was just working on cron_last conversion which I know will need a system_update_N function like many other state variables. I was just trying to be helpful from what I had already seen.

ACF’s picture

Status: Needs work » Needs review
FileSize
2.42 KB

Updated with variable delete added.

Lars Toomre’s picture

#9 looks good.

The only thing I would add is an inline comment that summarizes the thought in #3. That will help others who might wonder why there is no upgrade path for whatever was stored in that variable in D7.

ACF’s picture

Added comment, thanks.

ACF’s picture

Just seen the change naming convention for the variable and have changed the patch.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.71 KB

Looks good... just bumped the update to system_update_8028

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Great work, thanks!

Committed and pushed to 8.x.

webchick’s picture

Btw, that comment didn't wrap at 80 chars so I changed it to:

  // No upgrade path needed since the menu router will be rebuilt during the
  // Drupal 7 to Drupal 8 upgrade.

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