Problem/Motivation

NodeHooks1::formSystemThemesAdminFormAlter should go into NodeFormHooks along with the submission callback.

Proposed resolution

Move NodeHooks1::formSystemThemesAdminFormAlter and node_form_system_themes_admin_form_submit into NodeFormHooks

Remaining tasks

Review

Issue fork drupal-3532200

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

acbramley created an issue. See original summary.

acbramley’s picture

acbramley commented
Status: Active » Needs review

acbramley opened merge request !12474

acbramley’s picture

acbramley commented
Title: Consolidate form_system_themes_admin_form_alter and submit callback into NodeThemeHooks » Consolidate formSystemThemesAdminFormAlter and submit callback into NodeThemeHooks
smustgrave’s picture

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

This seems to make sense as other alters are in NodeHooks but imagine once the structure standard is decided this will have to be updated again.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

I'm not sure about this going into theme hooks.

Currently that class only has a preprocess hook in it - which means it'll be instantiated on render cache misses for nodes (quite often). Not sure if those requests already load ConfigFactory but they might not.

The theme form alter is on the other hand only called when you're on the actual theme settings page, which is very, very infrequent. Also while it has the word theme in it, is it really a theme hook as such?

smustgrave’s picture

smustgrave commented

Should this be postponed for when the node hooks are restructured?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I had the same thought, I'd probably put this in NodeFormHooks, even though there's only one of those in node module.

(FWIW, ConfigFactory is used a lot in bootstrap and rendering, _very_ unlikely that's not instantiated at this point.)

acbramley’s picture

acbramley commented

Should this be postponed for when the node hooks are restructured?

No, there's way too much bikeshedding in that, let's just agree on where these hooks should go.

I put them in ThemeHooks because it's dealing with theme stuff, but form hooks is fine by me.

smustgrave’s picture

smustgrave commented

Think should leave follow the structure did for taxonomy

nicxvan’s picture

nicxvan commented

The structure I proposed is they go in the file that has the hook is defined in. hook form alter is defined in form.api.php so NodeFormHooks.

Then we can make performance considerations and split out a specific hook if it's particularly expensive. We don't have a naming convention for that yet.

nicxvan’s picture

nicxvan commented

Also we don't want to use $this in form callbacks as @berdir explains here: #3514161: Better document how form submit callbacks should be called

acbramley’s picture

acbramley commented

Also we don't want to use $this in form callbacks as @berdir explains here

There's not a clear "right way" defined there? What should it be?

acbramley’s picture

acbramley commented

We could do $form['#submit'][] = [self::class, 'systemThemesAdminFormSubmit']; and then make it static and remove the DI?

nicxvan’s picture

nicxvan commented

Berdir understands this better than I do at the moment.

But as far as I understand you're right.

If the submit callback goes through class resolver then di would still work but I don't think it does yet.

There is another issue to handle that I think.

acbramley’s picture

acbramley commented
Issue summary: View changes
acbramley’s picture

acbramley commented

What should we do here then? Leave as is, or convert to a static callback without DI?

nicxvan’s picture

nicxvan commented

I think static callback like we did in file: https://www.drupal.org/project/drupal/issues/3534092

acbramley’s picture

acbramley commented

Done, should be good to RTBC now

nicxvan’s picture

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

I agree, looks great now.

catch’s picture

catch
he/him
Primary language English
commented
Title: Consolidate formSystemThemesAdminFormAlter and submit callback into NodeThemeHooks » Consolidate formSystemThemesAdminFormAlter and submit callback into NodeFormHooks
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Needs a rebase after another node hooks issue landed.

It would be good to open a follow-up to discuss whether there's a DI-friendly option for form callbacks. Could we use service notation maybe?

nicxvan’s picture

nicxvan commented

I think we can have that discussion in: #3514161: Better document how form submit callbacks should be called right?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

It would be good to open a follow-up to discuss whether there's a DI-friendly option for form callbacks. Could we use service notation maybe?

Thanks for the reminder, I had wanted to this, I think we just need to connect the dots: #3536726: Use CallableResolver for form callbacks

acbramley’s picture

acbramley commented
Status: Needs work » Reviewed & tested by the community

Just did a straight merge since the first commit in this MR conflicted horribly.

  • catch committed 44b24ad5 on 11.x 
    Issue #3532200 by acbramley, nicxvan: Consolidate...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

catch closed merge request !12474

Status: Fixed » Closed (fixed)

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