Rename ensure_exists to createIfNotExists, and camel-case simpleConfigUpdate for consistency

Created on 17 June 2024, 10 days ago
Updated 24 June 2024, 3 days ago

Problem/Motivation

Mixed-case methods make for a really poor authoring experience in Recipes. There's no reason why plugin IDs cannot use camel case to make for more standardized recipe creation as other methods are coming from directly mapped methods that will be camel case.

Steps to reproduce

Simple examples:
ensure_exists vs grantPermissions

Proposed resolution

  • Update recipe plugin ids to be camel case
  • Provide BC and deprecation notice to not break recipes in the wild.
Feature request
Status

Needs review

Version

11.0 🔥

Component
recipe system 

Last updated 1 day ago

Created by

🇨🇦Canada b_sharpe

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

Merge Requests

Comments & Activities

  • Issue created by @b_sharpe
  • 🇺🇸United States phenaproxima Massachusetts

    +1 for this. Having the cases be mixed is not good for the recipe authoring experience.

  • 🇺🇸United States phenaproxima Massachusetts
  • 🇺🇸United States thejimbirch Cape Cod, Massachusetts

    According to the config action documentation, there are two:

    simple_config_update
    ensure_exists

  • 🇨🇦Canada b_sharpe

    Thanks @thejimbirch, that was my findings as well. How should changes like this be handled to communicate to the documentation there? Is there a way to link the two?

  • 🇺🇸United States thejimbirch Cape Cod, Massachusetts

    We create follow-up issues to document changes/additions in the Recipes Initiative project to the 1.0.x branch.

  • Merge request !8435Resolve #3455113 "Recipe plugins ids" → (Open) created by b_sharpe
  • Pipeline finished with Success
    10 days ago
    Total: 572s
    #201127
  • Status changed to Needs review 10 days ago
  • 🇨🇦Canada b_sharpe

    Needs review, its passing tests, but not entirely sure about the deprecation/backward compat implementation.

  • Status changed to Needs work 10 days ago
  • 🇺🇸United States phenaproxima Massachusetts

    I don't see any problem with this; I think we probably just want some basic test coverage to confirm that creating an instance of the old plugins raises a deprecation message.

  • 🇺🇸United States thejimbirch Cape Cod, Massachusetts
  • Status changed to Needs review 9 days ago
  • 🇨🇦Canada b_sharpe

    So it appears the change record I was looking at is actually not yet committed. So instead, I've moved this to triggering a deprecation notice upon instance creation of the old names. Added a test for this as well.

  • Pipeline finished with Failed
    9 days ago
    Total: 276s
    #202285
  • Pipeline finished with Success
    9 days ago
    Total: 509s
    #202295
  • Status changed to Needs work 9 days ago
  • 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.

  • Status changed to Needs review 9 days ago
  • 🇨🇦Canada b_sharpe

    Test bot failed cause I pushed while running :S

  • Status changed to Needs work 9 days ago
  • 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.

  • 🇨🇦Canada b_sharpe

    b_sharpe changed the visibility of the branch 11.x to hidden.

  • 🇨🇦Canada b_sharpe

    b_sharpe changed the visibility of the branch 3455113-recipe-plugins-ids to hidden.

  • Status changed to Needs review 9 days ago
  • 🇨🇦Canada b_sharpe

    Jeez, 11.x instead of 11.0.x...

  • Pipeline finished with Failed
    9 days ago
    Total: 551s
    #202412
  • First commit to issue fork.
  • Pipeline finished with Canceled
    8 days ago
    #203648
  • Pipeline finished with Failed
    8 days ago
    Total: 679s
    #203649
  • Pipeline finished with Success
    8 days ago
    Total: 559s
    #203665
  • I have observed the test failures on the MR , which indicates still some files left in which plugin ID from ensure_exists to ensureExists needs to update.

    trace out those files, updated plugin id & test passed on local for me

    Updated files in MR as well , test failures fixed.

    Pipeline passed successfully & MR is mergeable now

  • Status changed to Needs work 8 days ago
  • 🇺🇸United States phenaproxima Massachusetts

    Thanks for working on this. We'll still need some light test coverage, per #9, to prove that the deprecated plugin IDs are still usable, but trigger a deprecation error.

  • I can see already basic test coverage added to confirm that creating an instance of the old plugins throws a deprecation msg in following file:
    core/lib/Drupal/Core/Config/Action/ConfigActionManager.php

    This can be check in this older pipeline, deprecation msg throws:
    https://git.drupalcode.org/issue/drupal-3455113/-/jobs/1897419

    correct me if I 'm missing anything

  • 🇺🇸United States phenaproxima Massachusetts

    You're right that the pipeline will show the deprecations, but that's not what we want. We need explicit automated test coverage of the deprecation, and we don't want the pipeline itself to show any, because that indicates that we're still using deprecated code in core.

    An example of how to explicitly test for a deprecation might be seen in \Drupal\Tests\system\Functional\Module\DeprecatedTemplateTest::testDeprecatedTemplate()

    (Basically -- your test method must be annotated with @group legacy, and you need to call $this->expectDeprecation(...) before you actually do the thing that will trigger the deprecation, which in this case is creating an instance of ensure_exists or simple_config_update.)

  • 🇨🇦Canada b_sharpe

    @phenaproxima, I had already added this here: https://git.drupalcode.org/project/drupal/-/merge_requests/8451/diffs#b4... is there somewhere else this is expected?

        $this->expectDeprecation('Unsilenced deprecation: The plugin ID "entity_create:ensure_exists" is deprecated. Use "entity_create:ensureExists" instead.');
    

    Also, I've been informed by @thejimbirch that this method is changing to `createIfNotExists` which likely means this issue needs an entire rework. I will check and see what expected order of operations will be here.

  • 🇺🇸United States phenaproxima Massachusetts

    this method is changing to `createIfNotExists`

    Yup, that's correct. In fact, let's just directly deprecate ensure_exists and replace it with createIfNotExists here.

  • Pipeline finished with Success
    7 days ago
    Total: 648s
    #204097
  • Pipeline finished with Success
    7 days ago
    Total: 1676s
    #204115
  • Status changed to Needs review 7 days ago
  • 🇨🇦Canada b_sharpe

    Oooook, rebased, changed and back to review :)

  • Pipeline finished with Success
    6 days ago
    Total: 631s
    #205042
  • Status changed to RTBC 6 days ago
  • 🇺🇸United States thejimbirch Cape Cod, Massachusetts

    Thank you so much; this is excellent work. Marking as RTBC.

    Regarding the name change. In a recipes leadership call earlier this week, we hashed out what ensure_exists really did, and want we want it used for.

    1. ensure_exists currently verifies that the config exists by it's id and nothing else.
    2. If the config exists it skips doing anything to the fields and moves on.
    3. If the config doesn't exist, it creates the config in Drupal with the values entered.

    This is a much more lenient approach to recipe dependencies vs. having config in the /config folder because it simply has to exist and not match exactly.

  • 🇺🇸United States thejimbirch Cape Cod, Massachusetts
  • Status changed to Needs work 4 days ago
  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    This is looking pretty good but the deprecation stuff needs a littel bit of work.

  • First commit to issue fork.
  • Pipeline finished with Failed
    4 days ago
    Total: 180s
    #206591
  • Pipeline finished with Failed
    4 days ago
    Total: 207s
    #206600
  • Pipeline finished with Failed
    4 days ago
    Total: 512s
    #206607
  • Pipeline finished with Success
    4 days ago
    Total: 541s
    #206627
  • Status changed to Needs review 4 days ago
  • 🇮🇳India ankitv18

    @alexpott updated the MR as you suggested, please review.

  • Pipeline finished with Success
    4 days ago
    Total: 631s
    #206801
  • 🇮🇳India ankitv18

    @alexpott updated the MR as per your suggestion.

  • Pipeline finished with Success
    3 days ago
    Total: 690s
    #206913
  • 🇮🇳India ankitv18

    Covered all suggestions and feedback, please review MR!8451

    cc:@phenaproxima @@alexpott

  • 🇨🇦Canada b_sharpe

    I'll let the others comment if this is cause for concern or not, but previously I was only resetting the cache if the keys didn't exist in the getDefinitions() method

    Do we care that now it is resetting the cache every call to this method instead of only when the replacements don't already exist?

        // This is no longer conditional.
        $this->setCachedDefinitions($definitions);
    
Production build 0.69.0 2024