🇺🇸United States @richgerdes

I also created a second followup branch which includes an additional commit to set the type hint. I don't think there is a branch for merging into Drupal 12 yet, so not sure what the next steps would be. I would expect that it would need to be broken out into its own issue and postponed until D12 is in development probably.

I tested this locally. Everything works as expected.

Converted the patch into a merge request and updated the depreciation message since we're no longer adding changes to 10.x branches.

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

Hi John,

I am still working with and supporting this module. I haven't gotten a ton of time recently to working on it, recently with a lot of other projects going on.

Are you in need of it for a specific project that you are working on? I can put some effort here to get some of my inprogress local work pushed out.

Please let me know if you have any specific questions about features or things that you are looking for. The module is built around flexibility and I am open to adding or extending it as needed.

Thanks,

Rich Gerdes

@nicxvan, I see it now. I guess i had opened 5326 instead. Thanks for clarifying.

I came to this issue via trying to clone products in commerce. Our implementation does use paragraphs as well for constructing the product pages. I still stand by hooks not being sufficient since there are cases where custom entities may or may not want to clone some items during this process. If you clone a product you might also want to clone the variations in that group and relying solely on a hook for this could get very complicated to maintain. Sure the hook does get us to a point that's usable, but i think switching gears in this ticket really hijacking this issue and repurposing the ticket. If we want to add the hook only as a stop gap, that should be done in a new separate issue, and this one should continue down the path that is was taking.

All in all, i do agree that a hook will likely be easier to get into core and does provide a minimum implementation, but I don't think it solves the request here as described in this issue and negates the work the I and other people have done to implement the full api

The code changes in the new PR have a lot of extra changes that don't all seem related to simply adding the hook. Does the diff seem right to you all?

I don't think only implementing a hook is sufficient to call this feature implemented. I think this really should have the full api that mimics that of creating an entity. The hook is the minimum require functionality to allow a module like paragraph to react to the duplicate action and clone its values as well. My use case for this was around duplicating products and variations within commerce, and I needed the ability to react on an entity level to cloning the product. Implementing most of this on an entity level lets module maintainers better handle this use case via the entity class and lets site builders handle the changes in a bundle class, without requiring code to live in. I initially started with the hook only to see if it was reasonable, but it quickly grew messy and I think that we would be doing ourselves a disservice if we don't provide a quality api for handling this operation. Doing so may actually eliminate the need to have custom modules handle cloning in the long term.

fathershawn → credited richgerdes → .

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

Hi @correktguy,

Can you provide a little more details on what you are trying to accomplish? Do you have steps to reproduce the issue you are seeing or can you provide screenshots of what you are seeing with some notes about how you would like it to work?

Thanks!

Hi @j-lee,

I definately agree that this is a good feature to support, but it relies on the functionality being supported by Webform, so we'll need to get the issue fixed there first then ensure that this module fully supports it. @jrockowitz worked on this in #3214847: Allow entity references in custom composite elements → . There is a PR there which partially implements this. If this feature is required, we should work on getting that pr finished.

I'm here with him at the Drupal NYC Contribution Day, and the general sentiment i that the feature opens a lot of questions and will include a lot of complexity for handling the saving, display, and rendering of the entity reference values, which introduces a lot of complexity and will call a lot of development. This can also be implemented via a custom composite class if its really needed.

@mistergroove, great!

Great work @smcgregor!

Hey @edwardsay,

Thanks for the feature implementation. I think this is a great idea.

Can you provide some more details on your use case? I think it sounds like you want to limit the number of items displayed in a list. It seems like since the availability of the announcements is determined dynamically based on the url and other conditions, you need to limit the number during selection/rendering to prevent too many items from being shown. Is this correct?

I think this makes sense as a feature, but I have a few questions.

1) Should the limits be set per block or per region. It seems like this likely should be moved to a block level setting instead of a region level setting, since regions can be reused by multiple blocks (say in a header and a sidebar) and you might want a different number of items displayed in each.

2) If we do think the limit should be kept, I think we should move the filter into the `loadActiveForRegion()` method. This might ideally be a new function (say getAnnouncementsToDisplay(Region $region) ) that takes the region as a parameter instead of doing this in the block. This will make the feature more reusable.

Hey @Droces, the module page already has the following under the Version 2 - Drupal 9/10 heading.

This module has been rewritten for Drupal 9+. The module provides a custom field-able entity type which can be displayed within the site.

I think this covers your suggestion. If you think we can make further improvements, please let me know.

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

I ran into this issue, it turns out this a compound issue that's the result of a few problems.

The item being added again when removed is the result of the order in which the facets are constructed and the processors execute. If the facets are not ordered correctly, it can result in the combined results not having the active state correctly updated. Just be sure that the facet you are combining is processed first (has a lower weight or is higher in the table then the second). In the example in #1, then issue would exist if the combined configuration existed on the animals facet (including trees). If the trees facet included animals, it should work perfectly. I've added a patch in !244 for ✨ Sort the facets based on the config in the facet summary block Active which sorts the facets when they are loaded in the facet manger. This mean that the facets are processed in a consistent order, and you can configure the facets to fix the mentioned duplicate parameter error.

The second issue I found here is that the facet's active state isn't updated when the results are merged by the combined facets processor. This results in checkboxes being correct, but the combined facet value only showing in the summary if one of the base facets is also enabled. This means that if just `lion` was checked, nothing would appear in the summary. If `lion` and `maple` are checked, both should appear. I've opened !245 to update the active state of the facet with the "combined" configuration during the build phase, so that the active state is reported correctly to the summary block.

You will need to apply both of these patches for the full functionality of the combined facets feature.

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

I ran into a similar problem where I noticed the facet list table sorting wasn't being applied to how the facets were being used. I think as a better solution, we should be ensuring sorting is used consistently.

I've opened a new patch which sorts the facets when they are loaded form entities, which should result in the sorting being applied consistently throughout the module. !244 has been opened for the new changes, and should hopefully replace the implementation here in a way that will provided better consistency.

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

You should not fork this module, instead follow the steps for taking over an abandoned project → .

if you have not done so, contact the existing maintainers via their Drupal.org profiles. If you can't or don't hear back, you can move the project to the "Project Ownership" queue and the project can be transferred by the admins. Please follow the instructions linked above.

This patch was committed.

https://git.drupalcode.org/project/uswds/-/commit/9d7f471a86696dd578389b....

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.