Remove unused file_file_video_presave()

Created on 30 June 2025, 3 months ago

Problem/Motivation

✨ Video media needs playsinline option Needs work introduced a file_file_video_presave() hook, but as @berdir pointed out:

but that's not a thing at all, because there's no such thing as a file_video entity type. It's also a legacy hook function.

I guess this should be a entity_view_display_presave() hook that checks for something using that formatter, but that's a lot of complexity for something as banal as fixing up a missing key in default config.

We have file_post_update_add_playsinline() post-update hook, so this seems to be dead code. Let's remove it.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

πŸ› Bug report
Status

Active

Version

11.0 πŸ”₯

Component

file.module

Created by

πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @kim.pepper
  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia
  • Pipeline finished with Failed
    3 months ago
    Total: 182s
    #535599
  • Pipeline finished with Success
    3 months ago
    Total: 634s
    #535602
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Seems straight forward.

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

    This isn't dead code as such, the upgrade path in that issue wasn't implemented correctly.

    When we're updating configuration, we need a post update that updates configuration in the site database, but also a presave hook that updates config that is shipped with modules/install profiles/recipes and triggers deprecations.

    See for example views_post_update_table_css_class()

    And

      /** 
       * Implements hook_ENTITY_TYPE_presave().
       */ 
      #[Hook('view_presave')]
      public function viewPresave(ViewEntityInterface $view): void {
        /** @var \Drupal\views\ViewsConfigUpdater $config_updater */
        $config_updater = \Drupal::classResolver(ViewsConfigUpdater::class);
        $config_updater->updateAll($view);
      }
    

    πŸ“Œ Add generic interface + base class for upgrade paths that require config changes Active would make it harder to get this wrong but for now we should fix this specific update to follow that pattern.

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

    Another possibility here is to ensure that the config is always valid with or without this config key. This is the current approach taken in πŸ“Œ Make menu trail behaviour in SystemMenuBlock optional Active (see the config schema in the MR) which avoids having any update path at all for existing config - that issue isn't in yet but it might be viable for changes like this that are adding something brand new that defaults to off anyway.

  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

    So it's just adding `requiredKey: false` under the playsinline block in the schema?

  • Pipeline finished with Success
    3 months ago
    Total: 626s
    #537626
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    So what's a recommended way to test this one?

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

    Going to go on a limb from looking at πŸ“Œ Make menu trail behaviour in SystemMenuBlock optional Active that the requiredKey is enough.

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

    Discussed this briefly with @alexpott.

    In πŸ“Œ Make menu trail behaviour in SystemMenuBlock optional Active the configuration form unsets the config key if it's FALSE (so it doesn't appear in the configuration) - I'm not entirely sure about that but whichever issue lands first is setting a precedent and we should probably decide on a pattern.

    Alex also pointed out that after this change, the original post update that was added also isn't necessary (because existing config will be valid), so we could consider emptying that update out.

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    +1 to #10 - plus we need to not set a default value in \Drupal\file\Plugin\Field\FieldFormatter\FileVideoFormatter::defaultSettings()

  • The Needs Review Queue Bot β†’ tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide β†’ to find step-by-step guides for working with issues.

  • Pipeline finished with Failed
    15 days ago
    Total: 428s
    #611809
  • Pipeline finished with Failed
    15 days ago
    Total: 828s
    #611815
  • Status changed to RTBC 15 days ago
  • Pipeline finished with Failed
    6 days ago
    Total: 770s
    #621088
  • Pipeline finished with Failed
    6 days ago
    #621105
  • Pipeline finished with Success
    6 days ago
    Total: 714s
    #621126
  • Pipeline finished with Success
    6 days ago
    Total: 3237s
    #621135
  • Pipeline finished with Success
    6 days ago
    Total: 1732s
    #621173
Production build 0.71.5 2024