Deprecate functions in menu_ui.module and move to hooks or helper class

I only found one issue and left a suggestion for it.

Personally, I'm OK with the use of procedural service calls here since there are already some to convert to DI in the Hook class. And it's the maintainer who is adding them.

I validated all of the code moves, checked the change record URLs, and grepped for use of the old procedural functions (see the aforementioned suggestion).

The only thing I see is we are using a combined change record when deprecations are simple like this.

We could update the cr links to https://www.drupal.org/node/3566774

And add the mappings of functions to method.

There are a few other crs that were created like this, we just emptied it out and changed the title to obsolete after moving the content to the shared cr.

We've been adding them to the CR before publishing so we don't forget, we add the link too so that people can see which ones are merged.

All feedback was addressed. The combined change record looks good. LGTM.

Technically they are services since hooks are services, but since it's called outside of the hooks I do think it makes sense to separate it.

I think this is a bit different. They're all form related callbacks and helpers, not an API.

I wasnt quite sure myself, because of the menu ui cache invalidation issue I'm working on, but I'm happy to think about some refactoring there if its beneficial

What i did consider is to suggest doing DI, always tricky with these issues

If the intention is to not publish the draft CR and only use the combined CR, then the URLs need to be updated. Also, the combined CR needs to be updated with the utility class.

I left some remarks in the MR

For me that’s new code. Then why not take the opportunity to modernize a little. At least we did that in all other conversion issues. But let’s hear other opinions

Opinion: Return types make sense, storage is already slightly tricky because it's in two separate if conditions, the code I would not refactor because it makes review harder with the special moved-code-diff from the meta. I don't think we "did that in *all* other conversion issues". Some were bigger refactoring partially because they were existing issues, but various other we try to keep changes as minimal as possible to keep it reviewable, such as the locale stuff.

In general the rules has been, we only do DI if the class already has DI or it is a brand new class.

For all new methods we add strict typing unless it's problematic for some reason.

We don't do code refactoring unless it's a pre existing issue or has pretty significant benefit and can't be done cleanly in a refactor.

I think this is ready again.

There is one open thread, but I don't think it's a blocker.

Some comments on the MR. I think it's kinda unfortunate this rickety menu UI code lives on as service code, but refactoring will have to happen in another issue in the future.

My preference is to move it. Or if it's easier, maybe we can leave it in the utility class, but mark it @internal. That way it's possible to for contrib to continue to use at their own risk, like the underscore function was, without increasing our API surface.

It's called in core/modules/menu_ui/src/Plugin/Validation/Constraint/MenuSettingsConstraintValidator.php though.

\Drupal::service(MenuUiUtility::class)->getMenuLinkDefaults($entity);

Hook services tend to have a bunch more stuff, I'm not a fan of doing that type of call with a hook class, it just feels wrong.

I'd prefer an internal utility class so it's isolated.

Looks good, this wasn't @godotislates's first choice, but I think internal it's much more preferable than adding it to the hook class.

Sorry, I thought I'd already committed this.

Applied a couple of my own minor suggestions of moving the @internal annotation on the MenuUiUtility class to just its ::menuUiNodeSave() method, since the other method is not internal. Also changed the service declaration to Drupal\menu_ui\MenuUiUtility: ~

Committed and pushed 5674bcd to main. and 41ea677 to 11.x. Thanks!

I reviewed and published the CR.