Replace "Expose all fields as blocks to Layout Builder" configuration with a feature flag module

Created on 22 March 2024, about 1 year ago
Updated 14 September 2024, 7 months ago

Problem/Motivation

To get around all the UX issues identified in πŸ“Œ [meta] Improve the "Expose all fields as blocks to Layout Builder" feature Active with this configuration page, we can instead use a Feature Flag ✨ Add an API for feature flags Active

This configuration is essentially being used as a feature flag currently, but does not need to be. The behaviour when this configuration is enabled is only there for BC purposes, it is not recommended for new sites to enable it. Therefore, we shouldn't be using configuration as this will lead to issues in the future when we need/want to deprecate/remove this behaviour.

Feature flags allow us to do this gracefully, and get around all of the issues with exposing this behaviour via configuration.

Proposed resolution

Remove configuration and form
Remove settings form tests
Swap configuration check with a moduleExists

Remaining tasks

User interface changes

Remove Layout builder Settings form.

API changes

None

Data model changes

None

Release notes snippet

N/A

πŸ“Œ Task
Status

Needs work

Version

10.3 ✨

Component
Layout builderΒ  β†’

Last updated about 14 hours ago

Created by

πŸ‡¦πŸ‡ΊAustralia acbramley

Live updates comments and jobs are added and updated live.
  • Needs documentation

    A documentation change is requested elsewhere. For Drupal core (and possibly other projects), once the change has been committed, this status should be recorded in a change record node.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @acbramley
  • Merge request !7142Replace config with feature flag module β†’ (Closed) created by acbramley
  • Pipeline finished with Canceled
    about 1 year ago
    #125747
  • Pipeline finished with Failed
    about 1 year ago
    Total: 167s
    #125750
  • Pipeline finished with Failed
    about 1 year ago
    #125774
  • Status changed to Needs review about 1 year ago
  • Pipeline finished with Success
    about 1 year ago
    Total: 497s
    #125792
  • πŸ‡¬πŸ‡§United Kingdom catch

    This is a very good idea and the diffstat also shows how much it simplifies things compared to using custom config. I think we should go ahead here, and whether or not we use feature flags for many other things can be figured out in ✨ Add an API for feature flags Active .

    We should have a follow-up to deprecate the module too.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    So moving the settings to flags how would that work for configuration exports and schema validation? Would flags be exportable?

    Change looks neat and can think of a few other tickets that could use this. Probably need to get in before 10.3 to avoid having to do a update hook to remove the config.

  • πŸ‡¬πŸ‡§United Kingdom catch

    @smustgrave in this case the flag is just whether a module is enabled or not - so it's all controlled by the extension config object with no new infrastructure.

    Couple of questions on the MR.

  • Pipeline finished with Failed
    about 1 year ago
    Total: 175s
    #127930
  • Pipeline finished with Success
    about 1 year ago
    Total: 613s
    #127934
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    +1 for more "zero config", which indeed requires more tightly focused modules to be installed. (A very similar example is how https://www.drupal.org/project/big_pipe_sessionless β†’ is a contrib module with zero settings that upon installation provides one very specific feature on top of core's big_pipe module. That is very similar to the structure/approach what's being proposed here.)

  • Pipeline finished with Failed
    about 1 year ago
    Total: 638s
    #129088
  • Pipeline finished with Failed
    about 1 year ago
    Total: 670s
    #129130
  • Status changed to Needs work about 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Seems to cause a few test failures.

  • πŸ‡¬πŸ‡§United Kingdom AaronMcHale Edinburgh, Scotland

    Just reporting the comment I just made on the parent issue because it's equally relevant here:

    Part of me feels like, if we go down the feature flag route, it still has the same problem that the current checkbox does, in that a on/off checkbox is not really the best way to communicating the two options. So I'm leaning more towards that, whether we have this as a feature flag or not, it may be better to still have a form with the two radio buttons.

  • Pipeline finished with Success
    about 1 year ago
    Total: 1078s
    #142351
  • Status changed to Needs review about 1 year ago
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    Fixed the legacy tags and it's back to green.

  • Status changed to RTBC 12 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Feedback appears to be addressed.

  • Status changed to Postponed 12 months ago
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    I don't see how this can be rtbc until ✨ Add an API for feature flags Active has been agreed. I'm +1 on the approach but there are still outstanding questions on the parent issue.

  • Status changed to Needs review 12 months ago
  • πŸ‡¬πŸ‡§United Kingdom catch

    I personally think we should go ahead here, and then if we add a new package, extensions page filter etc. in the other issue (or spin-offs from that issue), the module added here can be brought up to date with those when they happen.

    If we don't do this before 10.3.0, we have config to deprecate/remove in updates etc. to worry about.

  • Status changed to RTBC 12 months ago
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    @catch has convinced me that doing this now and before 10.3 is in our interest due to update functions. We can update the module's package etc later.

  • Status changed to Fixed 12 months ago
  • πŸ‡¬πŸ‡§United Kingdom catch

    Gave this another once-over.

    I agree we'll probably need to change this once ✨ Add an API for feature flags Active is resolved, but it'll be much easier to tweak the package and maybe hook_help() etc. than it will to make this change once the existing config has made it into a tagged release - we're saving ourselves 2-3 update hooks if we do it in this order as well as introducing then removing translatable strings etc.

    Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

    • catch β†’ committed 7e1b8996 on 10.3.x
      Issue #3432874 by acbramley, catch, alexpott: Replace "Expose all fields...
    • catch β†’ committed 41e2fd07 on 11.x
      Issue #3432874 by acbramley, catch, alexpott: Replace "Expose all fields...
  • πŸ‡¬πŸ‡§United Kingdom catch
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    Amazing! Thanks guys

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

  • πŸ‡ΊπŸ‡ΈUnited States luke.leber Pennsylvania

    Temp patch for 10.3.x that prevents a silly

    1. Site is slow
    2. Site is fast (after code deployed, before updb runs)
    3. Site is slow (after updb runs, before config imports)
    4. Site is fast (after config import turns the module back off

    deployment strategy 🀭.

    Intended use is to patch only during the 10.3 upgrade then remove. This can be helpful for users that consistently see database deadlock problems arising from huge block plugin cache writes...

    diff --git a/core/modules/layout_builder/layout_builder.post_update.php b/core/modules/layout_builder/layout_builder.post_update.php
    index ee735bf41b..6fd8891218 100644
    --- a/core/modules/layout_builder/layout_builder.post_update.php
    +++ b/core/modules/layout_builder/layout_builder.post_update.php
    @@ -74,5 +74,6 @@ function layout_builder_post_update_timestamp_formatter(?array &$sandbox = NULL)
      * Enable the expose all fields feature flag module.
      */
     function layout_builder_post_update_enable_expose_field_block_feature_flag(): void {
    -  \Drupal::service('module_installer')->install(['layout_builder_expose_all_field_blocks']);
    +  // It's silly to install this only to have config import uninstall it.
    +  // \Drupal::service('module_installer')->install(['layout_builder_expose_all_field_blocks']);
     }
    
  • Status changed to Needs work 7 months ago
  • πŸ‡³πŸ‡ΏNew Zealand quietone

    Changing to needs work and tagging because the related documentation updates have not been made. This change added a lifecycle link,
    lifecycle_link: "https://www.drupal.org/node/3223395#s-layout-builder-expose-all-field-blocks", but there is no information on the page for what to do if you are using the feature.

  • πŸ‡ΊπŸ‡ΈUnited States bkosborne New Jersey, USA

    With this feature module disabled, I'm seeing log messages like this when enabling layout builder on an entity view display of any entity type (tried both node and taxonomy)

    The "field_block:taxonomy_term:test:description" was not found

    It's just a warning and things are fine (probably after block plugin caches get invalidated somewhere).

    It's from BlockManager being unable to find the block plugin definition. I suppose there's some cache race condition here contributing to this. When you enable layout builder for an entity view display, it creates the default layout view display and places a field block plugin for each field. But since we're not exposing field block plugins until the entity view display is converted to layout builder.... yeah.

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    That's probably due to the fact that layout builder automatically (and blindly) adds fields to layouts even if the plugins don't exist. There's an issue for stopping that too

  • Attached a static copy of the proposed patch in #22 for those who need it.

Production build 0.71.5 2024