Jaipur, India
Account created on 22 December 2011, over 12 years ago
  • Staff Software Engineer in the Drupal Acceleration Team at Acquiaย  โ€ฆ
#

Merge Requests

More

Recent comments

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

narendraR โ†’ created an issue.

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

narendraR โ†’ made their first commit to this issueโ€™s fork.

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

Sorry, but I don't get #17. As per #13, Only lowercase alphanumeric characters and underscores are allowed, and only lowercase letters and underscore are allowed as the first character.

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

Adding _ in the end of field prefix failed core/modules/field_ui/tests/src/Functional/ManageFieldsFunctionalTest::testFieldPrefix. Either that test needs to be adjusted as per new validation rule or _ in the end of field prefix is not necessary. For now, I am removing the _ from field prefix end.

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

narendraR โ†’ created an issue.

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

narendraR โ†’ changed the visibility of the branch 3437325-add-validation-constraints to hidden.

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

narendraR โ†’ created an issue.

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

I think that is already handled in core/modules/field/src/Entity/FieldStorageConfig::__construct

if (!preg_match('/^[_a-z]+[_a-z0-9]*$/', $values['field_name'])) {
throw new FieldException("Attempt to create a field storage {$values['field_name']} with invalid characters. Only lowercase alphanumeric characters and underscores are allowed, and only lowercase letters and underscore are allowed as the first character");
}

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

narendraR โ†’ created an issue.

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

narendraR โ†’ made their first commit to this issueโ€™s fork.

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

Thanks for the follow-up. One test scenario which is missing here is "Imported node with owned by user that does not exist".
Rest LGTM.

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

A follow-up documentation issue might be required to help understand how content can be created/imported using recipe system.

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

A follow-up documentation issue is needed to include this change. Rest looks good to me.

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

I think follow up needs to be created to update the recipe documentation to include this feature.

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

Re #7, Test started failing after reverting the changes.

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

Re-rolled the code and changes done as suggested. I am not sure what needs to be done next here as ::testValidationAfterRecipeApplied is still not failing as expected.

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

Overall changes in code looks good to me and can be marked as RTBC, but I have a doubt in test implemented.

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

Re #15, I tried adding debugger in CKEditor5::validatePair, but that is not getting triggered. May be something is wrong in this MR's core/tests/fixtures/recipes/basic_html_format_editor/recipe.yml

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

While working on this issue it seems that the failing test is already handling the PluginNotFoundException.
https://git.drupalcode.org/issue/distributions_recipes-3423523/-/jobs/94...

Not sure if the changes I did are required or not.

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

I think #3348991: Add a method to add configurations using recipes. โ†’ is also same issue. If that is the case, we can close that issue as work is already started here.

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

Status retained after merging latest code from 10.2.x branch.

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

narendraR โ†’ changed the visibility of the branch 3401723-config-modified-by-rebased to hidden.

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

There are basically 2 things which needs to be validated.

  • config:actions
    config:
      actions:
        config_test.dynamic.recipe:
          ensure_exists:
            label: 'Created by recipe'
          setProtectedProperty: 'Set by recipe'

    config_test should be either already present or is defined in recipes. And I think if action start with core, we don't have to do anything. eg in case of core.entity_form_display.node.article.default

  • config:import

    config:
      import:
        config_test: '*'

    config_test should be either already present or is defined in recipes.

Not sure if this issue should handle both as suggested in MR comment, added IS update tag for that.

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

narendraR โ†’ made their first commit to this issueโ€™s fork.

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

Re #11, entity_create:ensure_exists, simple_config_update is what I referred. Should I change this?

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

I am not sure if configfield.storage.node.meta_tagswould already exists or need to be created when config action is applied. I think it will be present.

From the config file I need to get entity type which will be used to get the related bundles. From bundles lets say article and page, I need to create field.field.node.article.meta_tags and field.field.node.page.meta_tags. Is this assumption correct? What if one of the field already exists?

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

This MR still has phpstan issues, but wanted to make sure if this is heading in right direction.

๐Ÿ‡ฎ๐Ÿ‡ณIndia narendraR Jaipur, India

I added ConfigImportKeyExistsConstraint to 'install' and it is working now ๐Ÿ˜ฌ. Not sure if this is the correct place though.
If this seems correct then we might expand ConfigImportKeyExistsConstraint to validate other things like whether to import some, all, or none of the module's config.

TBH, I don't know if recipes support config:import none.

Production build https://api.contrib.social 0.62.1