🇺🇸United States @richgerdes

Updated description formatting.

So we can add functionality to EntityInterface if we want to.

While we can add new apis, it's better to not add BC breaks if we can avoid it. Similarly to the entity class, we are adding a function to the EntityStorageInterface too, which is also a BC break. We probably want to add an interface that add the create function. It would be nice to see these two new interfaces be removed in a future version of Drupal core (probably not 11 at this point, but 12 maybe?).

Is the pre/postDuplicate stuff necessary?

Sure, technically a hook is all that's needed to cover the basic description, but when I ran into this issue, i also wanted to check the duplicate state at save time, which isn't covered by just using a hook unless I create those properties myself during the hook.

Both the config entity base and content entity base make use of the pre/post hooks. We could move that logic into the storage class, but I would recommend against it. If the api is setting an expectation for what will happen with an entity, then the logic should live on the entity class, even it its more functions. IMO the storage service should deal directly with loading entities, and not with managing entity properties field data. This is the best way to ensure that data is updated correctly over time.

Moving this logic to the entity storage class is a bad idea because a developer could override the storage class with a custom implementation. While we hope that the class override extends the existing base storage, we can't be sure that it will and that means the new class might not include the same logic for updating the entity during these events and for the duplicate operation to work consistently, then entity class is the best place for the logic.

The other question is what does core already do? The entity classes already have pre/postSave() and pre/postDelete() functions, so there is precedence to have a similar api structure for duplicate. Additionally, since the duplicate process is similar to the create process, which would call the entity's constructor, we should have a way for the entity class to react to that as well, no?

On the flip side, there is a lot of logic in the existing content/config entity storage base classes as well. For example both createRevision() and createTranslation() exist on the ContentEntityStorageBase . Given the possibility of overriding the class the field value updates should probably be moved to the entity class instead, unless there is a strong need to tie the logic into the storage service such as loading the new serial id from the database. Given the current logic for these two functions, the main reason we need to use the storage handler for entity operations is to expose the module handler service in order to invoke info and alter hooks. If we want hooks, this may be unavoidable, but the hooks could be run along side.

Given the above, I would still couple any entity data changes as closely to the entity class as possible, and for translation and revision support, that should likely live in the relevant traits instead of the generalized storage.

I am for simplifying logic where possible. Right now during clone the following happens.

1. User calls EntityBase::create()
2. That gods to EntityStorageBase->create()
3. We then call EntityBase::preCreate()
4. Then EntityStorageBase->doCreate() which calls EntityBase::__construct()
5. EntityStorageBase->create() then calls $entity->enforceIsNew()
6. Folloed by
   

    $entity->post create() ?</li>
     <li>Finally the "entity_create" hook</li>
   </ol>
   
   This flow is pretty lean, and doesn't expose any pre/post operations on the storage class itself, so either we would need need to add support for pre/post functions there, or a developer would be forced to override <?php EntityStorageBase->create() 

   to alter the logic, which seems more messy then adding a few extra functions to the process ahead of time. Maybe its not necessary, but seems like its a better Developer experience to provide the apis up front, which is what I think this patch is currently doing to mimic the create flow.

   What is the use case for the getDuplicateSource() and related methods?

   I think the duplicate source is a valid thing to keep track of during the duplication process. Sure this could just be a variable passed to the classes, but I think it makes sense to store it on the entity level as a temporary value, in order for use of the "duplicate" state outside of the hooks. For example would be nice if a module wanted to change something in the form during load, based on the duplicate state, there is no way for it to identify that without the class property/function.

   What's left?

   Skimming the comments, it seems like the last major issue outside of the above discussion is that "there is also no new test coverage for the hooks yet". Is there anything else that still needs to be done here?

@Mmillford, #60 should have a D10 patch that hopefully works. Probably need to test it though. The !204 MR has been rebased and remapped from D9, so that's likely where the extra commits came from.

Changes look good! This should be all set to go out. I confirmed the only issue reported by rector was updating the compatibility.

Thanks for the patch and review.

I merged the work from 🐛 Throwing error in Plain drupal site Needs review which should resolve this. @aman_lnwebworks, please give this a test if you get the chance, and let me know if you still have issues.

Tested Changes locally. Looks good.

@AndyD328,

Did you ever get this resolved. Looking at the releases, 1.0 RC11 should have D10 Compatibility → . Are there errors you are seeing when using the release?

The module shouldn't be suppressing any of the entity fields. The view you select should render underneath the rendered field entity, which means you should be able to use the entity display configuration to setup the display of the header to include any fields, such as the description.

richgerdes → created an issue. See original summary → .

richgerdes → created an issue.

MR 7 provides an alternative approach that breaks apart the value sent by the form submission. The old logic hard coded the menu name name to main, but didn't successfully remove the prefix from the menu items if the menu name was different.

richgerdes → created an issue.

Two sub modules not exist which handle adding events to group, and assigning groups to manage events. This patch is no longer required.

https://git.drupalcode.org/project/event/-/tree/8.x-1.x/modules?ref_type...

This was fixed a while back. Thanks for the patch!

https://git.drupalcode.org/project/event/-/commit/323effa354dd9d67b6df5c...

richgerdes → made their first commit to this issue’s fork.

Merged the change so I can look at D10 Compatibility.

Typically a clean commit should be as simple as possible, only changing the lines required to fix the bug. In this case the MR changes the line endings of the install file form the unix format (only \n) to the windows format (\r\n). This is an easy enough to ignore in the editor but it does clutter the git log and make it harder to trace the cause of a bug in the blame history. Best practice is typically to make a change like that as its own commit.

In this case, its not necessary and would deviate from the established standard of the repository. I've reverted the line endings changes in a new commit on the branch.

Otherwise, I think that the two hooks being added are good, and will benefit users of the module. As @lostcarpark, event_update_dependencies is more of an informational hook, instead of a update process, so its ideal that they are kept together. I've also moved the change to the top of the file.

Otherwise everything looks good to me. @tra.duong, I really appreciate the through and thorough work here to get the hook added.

Reviewed the MR for this. The breaking change was that hte config_export key was not defined, which was addressed in #3295335: The Event Type Entity Definition is missing the "config_export" key → . The other changes here are not required. I'm closing this as a duplicate of #3295335: The Event Type Entity Definition is missing the "config_export" key → .

richgerdes → made their first commit to this issue’s fork.

richgerdes → made their first commit to this issue’s fork.

-

Hi all! Thanks for the review here.

Since the last change to dev was only the D10 compatibility, I've opted to make a stable release for the 1.0 branch instead of adding another RC.

The release is now available: https://www.drupal.org/project/openapi_ui_swagger/releases/8.x-1.0 →

Let me know if you need anything else.

Thanks for the patch! Looks good to me. I've merged it to the dev release

richgerdes → made their first commit to this issue’s fork.

Thanks for the patch! I tested it and it looked good. I committed it and have tagged a 1.0.1 release → with the changes.

Thanks for the patch and MR. I tested it and it looked good. I've merged the MR and have tagged a 1.0.1 release → with the changes.

The two above branches are functionally the same.

https://git.drupalcode.org/issue/commerce_google_tag_manager-3340566/-/c...

I think MR 7 follows the code standards a little closer, and probably should be the candidate merged.

+1 for MR1.

I had this problem when upgrading an older site. Turns out it was the result of the crop module not being installed, but its entities were still in the definitions table. I was able to reinstall crop and then uninstall it via Drush and that cleaned up the extra definitions.

I'm still seeing this issue on Drupal 9.5. Patch from #11 did not fix the issue for me, but the patch from #5 did. Not sure why the new patch didn't work. Some investigation is needed.

My use case is a modified version of the commerce cart as well which uses ajax to update the quantities when they are changed. I'm seeing the action of the form change when the views form is replaced.

Updating the target to 11.x since that's the new standard and this code looks unchanged since the 9.5 release.

richgerdes → made their first commit to this issue’s fork.

volkswagenchick → credited richgerdes → .

richgerdes → created an issue.

richgerdes → made their first commit to this issue’s fork.

richgerdes → created an issue.

@nagy.balint, thanks for the quick review.

I'm not 100% sure the bug here is the fault of the condition_field module itself. Seems like some odd behavior in core that isn't mapping columns the database to field properties well. I probably will dig into it further, but i think our use case here is pretty unique. I suspect that its rare an array is the only property in a base field, which is why the existing core code didn't handle it.

Also thanks for the code review on the module and going above and beyond. I had been meaning to fix the form settings, but kept forgetting. Should be fixed now. Thanks for testing it out!

For anyone following this, I've released an alpha version of a 2.0.0 release for Drupal 9 and 10.

I've opened an MR with a fix for this. This processed the default value on save to ensure that the value is checked for the existence of the "conditions" key. If the key isn't found, then the module will nest the value under the key. This is also written in a way that supports any modules which might want to extend this field type and add additional properties.

richgerdes → created an issue.

Patches look good.

I've opened an MR for these changes.

richgerdes → made their first commit to this issue’s fork.

This is fixed in commit 1f27b5eadb0b6b5853ebd6ce94b1418a7382bd96.

You can fetch and apply the patch from the link below

https://git.drupalcode.org/project/fastly/-/commit/1f27b5eadb0b6b5853ebd...

It appears this is actually fixed in the dev release, however the dev release is not installable on Drupal 9.

richgerdes → created an issue.

@NancyDru,

Thanks for the response. I am excited to move forward with the project.

I'll definitely update the homepage with more info once the project is further along. I'd be open to implementing an upgrade path for the Drupal 7 version to the new one, but I think that will depend on how the functionality works out.

I have mixed opinions on this. I personally have a preference for the select list over the auto complete. I see the desire when a site has a lot of views, but I don't think it should be the default. Ideally for a simple site the select list is a better user experience. I think that auto complete here is unnecessary in most use cases and feels like a less stable implementation. Generally when I've run into an issue like this, I've used a module such as chosen → to switch to an auto complete interface leveraging the front-end instead of requiring the ajax call to load options. Is there a particular reason why you think that the auto complete is a better UX here?

Gerhard,

Thanks for the support. Happy to have you involved if you want to be part of the build. I have started a draft of the module locally, but would like to get it added to a repository before too too much more work.

Would you be able to add me as a maintainer for the project?

Thanks,

Rich

richgerdes → created an issue.

@tra.duong, i looked at your MR, but its hard to tell what was actually changed because you reset formatting on the entire file. Would it be possible for you to split the MR up into two commits, one to include the general formatting updates and one for the update hook you added so it can be easily reviewed?

richgerdes → created an issue.

I'm also adding #3182180: No support for defining a default value for a field of a composite → to the list of things to add. I think we should address that as well.

I did just release 1.0-rc3 → today, which should address the biggest concerns, adding a few of the RTBC and bugs with patches along with Drupal 10 support. If there are no major issues, I think that should be the last RC and we should make a full release soon. I would like to see at least #2928938: Prevent editing/deletion of inuse composites. → solved before we have a full 1.0.

@joseph.olstad, the main goal of this module is to provide a complex web ui configurable element, which can be reused across multiple forms. The base module does provide the idea of a composite, and lets the user build one with code, but this isn't ideal for non developers, which is why the module was born. I had hoped it might also be the home to other webform related tooling if anything helpful came up in the future.

@svendecabooterm, thanks for interest in helping maintain the project. I would be open to adding someone if you are still interested. I haven't been using this module as heavily recently so haven't gotten back to it as much, so another set of hands who's actively using it would be a great addition. I'd still be open to help if you want to. I'm not always looking over issues or the emails for them, but feel free to ping me on slack if you are still interested and I don't get back to you right away.

Looks like some great collaboration here! Sorry I haven't gotten to committing this sooner. Thank you all for you hard work!

I've committed it and released it as part of 1.0-rc3 → .

Thanks everyone for the hard work getting this fixed and testing the patch. Everything looks good to me. I've committed it and released it as part of 1.0-rc3 → .

@bgilhome,

I think these template suggestions make a ton of sense. This patch looks good. Thanks for the hard work! I've committed it and released it as part of 1.0-rc3 → .

@Sweetchuck,

Thanks for the suggestion. This change looks good. I've committed it and released it as part of 1.0-rc3 → .

@Webbeh,

Do you know if this is supported by the base webform's composite functionality? When the module was built, I don't think it was. I don't believe we are doing anything to prevent the use of any fields within this module, and just rely of the webform modules list of available composite sub element options, so if the base module has support it should just work here too.

Let me know if you are able to add a entity to a composite in a form and not to one of the global composites provided by this module, and I can look more into it.

richgerdes → made their first commit to this issue’s fork.

Thanks for the fix! The changes look good to me. I've committed this and released it in 8.x-1.3 →

@codebymikey, Excellent catch there. Thanks for the fix! The changes look good to me. I've committed this and released it in 8.x-1.3 →

richgerdes → made their first commit to this issue’s fork.

@DishaKatariya,

Thanks for the review and testing. I've committed the patch and released a 1.2 version → .

richgerdes → created an issue.

@AndyD328,

I've been busy, so I'm a little be hind on D10 releases. I've committed the auto generated patch dev, and marked the project compatible with D10. I'll do some testing, but I'll hopefully make a versioned released in a few days. If you want to test the dev release and let me know if you see any issues, please feel free and let me know if you notice anything.

Sorry for the spam. I rebased the branches so that I could roll a patch that applies against 9.4.x, instead of 9.2.x, but forgot that I don't have access to change the MR target branch. Someone will need to update the target branch to 9.4.x so this can be better reviewed....

In the mean time, I'm posting a patch here for reference. This should apply fine.

I've taken a pass at improvements here. I've updated the workflow for the createDuplicate() function to mirror that of create(), and making a call out to the storage service to handle the duplication. Similarly to the create() on the storage class, this implements a createDuplicate() invokes preCreateDuplicate() on the entity class, then calls doCreateDuplicate(), and ultimately calls $entity->postDuplicate() before invoking new hooks (ENTITY_TYPE_duplicate_create and entity_duplicate_create).

I've opted for the name `duplicate_create` for the hook, which follows the same format as `revision_create`. Additionally new apis for getting/setting the duplicate source entity and checking if the entity is a duplicate (like isNew) are included now.

richgerdes → made their first commit to this issue’s fork.