🇸🇮Slovenia @deaom

This seems to be corrected in the message subscription email module 🐛 "LogicException: An anonymous user must be identifed by session ID." Active also as mentioned in comment #12 🐛 An anonymous user must be identified by session ID (line 176 -> line 223) Postponed: needs info , not a Flag module issue, so can be closed as it works as designed.

Added additional field link duration that applies when ajax link type is selected, so the user has actual control on the duration of animation. That value then overrides the animation duration via JS. Updated test to now cover the added button (X). Needs review.

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

The issue happens as the JS code can't append the flagging message, in this case Thank you, to the flag link as there is none on the page. There is also a console error that shows that. As the permission to unflag is not there, the markup is not generated and therefore the message not appended. Updated the code so the markup is displayed without the unflag link and then removed in the JS. Not sure if the right approach, so needs review.

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

I do think this works as designed, as the "pop up" that is shown is the confirmation dialog when clicking delete that is present across Drupal (even when deleting a node, the pop up with the are you sure is shown).
Steps to reproduce:
Create or edit a flag settings (when using the provided bookmark it would be admin/structure/flags/manage/bookmark)
select Link type as Field Entry Form and Form behaviour as New page.
After saving the new setting go to content and flag it, now unflag it/Remove bookmark. The edit flagging details page will open with the option to update or delete the flag. When clicking delete a new confirmation pop up appears where user can delete the flag or cancel deletion, which is Drupla behavior for deleting entities. My POV works as designed.

I assume it was the easiest way to target the links as the for loop is already there. Did remove it from JS and added it directly in flag twig. Needs review.

Based on the comment from Berdir [ #18 🐛 Invalid CSRF token using flag.link_builder service Needs work ] not sure if the MR is the right approach?

Like mentioned in comment #4 this is already part of the module and works. When filter is exposed to visitors, the "required " check box needs to be removed. If the filter is not exposed to visitors, the default value is set to true. Don't think that changing it to All does anything.

The thing that this MR changes is the default value is no longer true, but All. Everything else stays the same. So not sure if this needs to be merged at all. Will rebase branch in case the desire is there to merge it.

Re-based branch, tests are now passing.

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

Was already confirmed as working, re-based branch, tests are passing.

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

Added additional check to not create a new flagging if it already exists. This then mean that if link is copied the edit dialog will be displayed, this preventing multiple flags being added to the same node. Needs review.

deaom → created an issue.

It is a feature request to allow multiple flaggings of a node by the same flag.

Based on the steps to reproduce and also manual testing, this is also a bug discovery, that would be good to correct.
In the steps it mentions UI will not allow it, so copy the link, as UI returns exception/error, that entity was already flagged. Doing it by copy and pasting the link is the bug discovery, as the check is not performed. That would need to be addressed first, as it's not how the module currently works.

I've tested the changes in MR and they do work in a sense that you can remove multiple flaggings, but it still does display just one link for "unflag", so user does not know what is being unflaged as there is no selection to it on a node. The same issue of flagging still remains, the UI does not allow multiple flaggings, you need to copy and paste the link to do it (once the check is added, that will not be possible anymore). Once you flag the node, there is no option to flag it again as you get the "remove" link, so the option of multiple flagging is not added with this MR.

If you are on the view of all flags, like my bookmarks, you get the separate links to unflag specific flag and that does work.

Also this is only working when the Link type in flag is Field Entry Form, does not work with other links (not sure if that was the intention).

Tests are passing, branch is rebased, so marking it as RTBC.

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

I can't reproduce the issue, as this is an old issue, assuming it was fixed somewhere?
I've enabled the default flag bookmark for an entity type and enabled the layout builder for the same entity. Visited the node layout and added a block flag bookmark. The flag is there and it work, and there is no error in console. Tested with D11 and flag 5.x. From my POV can be closed.

This one I think was solved in 🐛 Clone form is not respecting 'cloneable_entities' config resulting in memory limit on recursive in ContentEntityCloneFormBase for a custom Entity. Active . As I'm not sure leaving status as is, but can we please use MRs for better management of the code changes.

This is already corrected in dev version with the issue 🐛 EntityCloneCloneableField isClonable fails when target_type setting is missing Active . So closing as a duplicate.

Updated the test to set global tag separately and removed the "switching to global tag". Needs review.

deaom → changed the visibility of the branch 2870574-test-should-not to hidden.

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

The issue mentioned is no longer present (it is a 3 year old issue). The labels can normally be translated via translate tab, even when the option for link is confirm form.

The code in constructor of FlagLink twig extensions seems to be some legacy code, so rather then removing it, I've added the ignore next line for the Dependency Injection issue. Ready for review.

Added test for twig extension, leaving to Needs work as phpstan for dependency injection still needs to be updated in the FlagLink.php

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

Updated the UserFlagTest to check that the relationship is not present on initial install and is present without additional cache clear after the user flag is set.

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

Rebased branch and solved conflicts. Tests are passing, from my POV can be merged but leaving status to Needs review.

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

Can't change the MR branch to 5.x so rebased to 8.x with updated tests that are passing. Needs review.

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

This issue seems to be opened to document the removed code in help hook. The removed code in help was for the Flagging form → module, which seems is no longer supported or used? So this issue can be closed as no additional changes are needed.

The type=button is added to links and no functional change or style change is happening, so can be merged. Do not have enough permissions to change the target branch in the MR, so that needs to be updated before merging, but did rebase to latest 8x. Setting status to RTBC.

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

I think the tests currently present already cover this scenario, they are added in the main module, so setting status to Needs review if additional test are needed, please provide information on what needs to be tested.

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

Updated the t() and rebased the branch. From my POV can be merged, but leaving status as is as.

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

This is actually working as expected based on code and test. In test FlagBookmarkUITest->testUiBulkDelete() a user is created with the following permissions:

• 'flag bookmark',
• 'unflag bookmark',
• 'administer flaggings'

Anonymous users can't use bulk form actions if they do not have correct permission set. Anonymous users need permission for flaggings, so "Administer Flaggings" permission needs to be set to allow use of bulk form for "Delete flagging". The removal via the Remove bookmark link works as expected on the bookmarks page.
Leaving status as is, but basically works as designed.

Removed the logic from .module and added and updated it to the form for the entity clone settings page. Needs review.

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

Test are now passing, ready for review.
There was an issue where PTs were getting cloned even if excluded, an issue that was already solved in 🐛 Clone form is not respecting 'cloneable_entities' config resulting in memory limit on recursive in ContentEntityCloneFormBase for a custom Entity. Active . The code for PT handling is from there, adjusted for this code.

After re-basing the tests are failing. It does seem that the circular cloning no longer works as it does not clone that node, just references it. Looking into it, but do not mind if somebody will be faster then me.

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

Test is added.

Could confirm the issue based on the reported steps. The cloned node is populating paragraphs from existing translation instead of the original paragraphs. After applying the MR, the paragraph in the cloned node is populated with the original language content. It does not affect if the checkbox is checked for "Save cloned content with all translation".
From my POV can be merged, but as this also changes the same file as the other issues, there will be conflicts that will need to be resolved. Leaving it up to the maintainer on how to handle the order of merging (as there are older issues that change the same file).
Maybe a test that tracks this change would also be good to add. Leaving it up to maintainer and leaving status to needs review.

Tests are now passing. Please do review if everything is still working as expected as there were conflicts when rebasing, so there is always a chance something was not resolved correctly.

Needed to squash previous commits to be able to do a rebase as git does not like commit messages starting with the #. Did a rebase and corrected some immediate phpcs issues. Now onto tests. I'm not sure everything was rebased correctly as there were some changes happening for the cloning of bundles and because of that conflicts. I'm looking into it, but do not mind if somebody else gets to it before me.

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

Tests are failing, so that needs to be corrected.
Wanted to point to three issues with entity reference/paragraphs and content translation that already have a possible solution 🐛 Translatable entity reference fields are not cloned correctly Needs work , 🐛 Corrupted data on cloning entity that contain paragraphs Needs work and 🐛 Content moderation: Check if entity is translatable Active that might already solve the reported issue.

Added a test module that creates a new content entity "book". Additional test then checks that if book is not marked as cloneable and added as a reference to a node that is cloneable, it's not cloned when the parent node is cloned.

Strange behavior in test as if I manually test with the same permissions with authenticated user all works normally. If the $field_definition instanceof FieldConfigInterface check is removed from isClonable function (EntityCloneClonableField.php), then the tests are passing, but when testing manually you then also get the revision_uid, uid etc fields (like mentioned in comment #12 🐛 Subentities referenced by BaseFieldDefinition field are not duplicated Active ).
I'll leave the status to needs work if anybody else sees what I'm missing.

deaom → changed the visibility of the branch 3179688-subentities-referenced-by to hidden.

deaom → changed the visibility of the branch 3179688-recursively-cloneable-base-field-entity-references to active.

deaom → changed the visibility of the branch 3179688-recursively-cloneable-base-field-entity-references to hidden.

The failing test is for previous major version (should be D10.4.5), D11 is passing. Because of that setting status to needs work.

The issue was not merged, so you either need to test the MR or create a patch from it and apply it in your project and then test.

Branch re-based so action tests are now passing.

Branch is re-based and tests are passing. Not sure if any additional test are needed/required so leaving status to needs review.

Branch re-based and tests are now passing.

The branch was re-based and tests are passing. Not sure if any additional tests are needed so setting the status to needs review.

Branch is re-based, tests are now passing, ready for review.

Branch is re-based the action test is added back in and the tests are passing. Leaving status as is, but from my POV can be merged.

Tests are now passing, there was also a manual review. Will leave status to RTBC, but fell free to change if needed.

Test are now passing, but this really needs to be checked and tested. The issue with tests not passing was with the referenced entities specifically in this test case paragraphs, as even if they were marked as not allowed to clone, they were getting cloned. With the code changes, they are no longer getting cloned, but rather referenced as it was before (or still is in the 2.x without this change). I'm not sure the logic is correct and if it breaks anything as I'm not sure tests are covering everything, so it would be good to test manually and also check the solution is suitable.

Updated the test and re-based the branch. Tests are passing, setting status to needs review.

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

I'm not sure I understand the issue (or maybe it was solved in the meantime). When I go to clone a content block, there is the option of "Save cloned Content block as published", if that is checked the cloned block is published. The core is handling the publish/unpublish checkbox field (status is there, just not added to UI). If the checkbox is not checked when cloning, once the core issue will be merged, the status will be possible to update/change, until then, don't think this falls under this module as the status can be updated in code for the specific block (or block can be removed and cloned again with the publish option checked).

The action test that is causing test failures is still removed here, but should be added back in once 💬 Add action module as dev dependency. Active is merged.

deaom → changed the visibility of the branch 3168732-allow-to-alter to hidden.

Added a check as when you first save the settings, the form_settings are empty/null, so array_merge returns an error, as the first element is null (TypeError: array_merge(): Argument #1 must be of type array, null given in array_merge() (line 119 of /contrib/entity_clone/src/EntityCloneSettingsManager.php).) and thus preventing the settings form to be saved.
After the first save, the form_settings is populated.
This can be tested by installing the module and saving the settings, or if the module is already installed first uninstalling it and then installing again and saving the settings.
Leaving to Needs review, but based on the comments can be merged.

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

Ready for review, once 💬 Add action module as dev dependency. Active is merged, test should pass.

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

The issue 💬 Add action module as dev dependency. Active needs to be merged for action tests. I've applied and committed the patch and opened the MR. Leaving status to needs review.

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

I'm not sure if this is a bug or works as designed, so leaving to needs review. Can confirm that the steps to reproduce and then switching to MR show the described issue and fix for it, I'm just not sure what's the correct behavior here. One would assume if the config is saved and then that is overridden it's overridden for a reason and probably should be saved to the form and displayed to the user. So that the user does not re-save without the override. Like I said, not sure what's the correct approach here, so leaving to needs review.

Issue 💬 Add action module as dev dependency. Active needs to be merged for failing action tests. Applied the changes from #13 patch so it's all in a MR. Leaving status to needs review.

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

The tests are failing because of the action module no longer being in core, once the issue 💬 Add action module as dev dependency. Active is merged, the test should be re-run and possible test issues solved.
The permissions are now removed from the admin/people/permissions list, based on the clone entity settings form. Needs review, so leaving status as is. Branch is re-based.

The way this works with the MR is, in case a role has a permission set - Clone all content entities and on the clone entity config form the clone content is disabled, the role that had a permission no longer has it, it's removed. The permission Clone all content entities is still listed under /admin/people/permissions and can be set, even if the clone content is disabled. If the configuration then gets re-saved it is again removed from the role.
I'm not sure if permission can actually be removed from the admin/people/permission, but it would probably be good to also check on the save permissions if the configuration to clone that entity is set, and if not, to not allow re-setting the permission, as the clone config form is usually not saved multiple times.

Maybe I'm not quite understanding the issue or request here, but the clone option is added to the Operations in admin/content view, where a node can be cloned. The clone "button" is also present when on node (next to edit, delete, revisions - if you have the permission of course) and the clone option was also added to the contextual links. What does this change actually do (except provide a field in view) and is it really needed?

I don't think there is an issue with the UniqueField constraint and cloning, as when that is set to something that is not uuid, it works as expected.
Tried setting the UniqueField constraint to user as in author, and that gets cloned with no issue (the user gets the cloned suffix added). When adding a content if the same user/author is added, it flags it as an error because of the constraint, meaning constraint is active.
I think in this example the issue is in the uuid and how Drupal handles it. Also setting the field to read_only probably does not help in overwriting with a new value.
Leaving open for somebody else to also have a look.

All tests on other issues are failing because of the action test (action module was removed from core). As this seems to solve the issue based on the green tests, it would be good for this MR to get merged so other tests can be re-run. Before merging a re-run of test would probably be good. Setting to RTBC.

This seems to be a duplicate of the issue 🐛 EntityCloneCloneableField isClonable fails when target_type setting is missing Active which has a solution. Closing it as a duplicate.

As far as I'm aware, this is not possible. The cloned entity type is "cloned" from the original, so you can't clone article into page as the code does not change content type.

Actually changing status to RTBC.

Based on the comments this issue is RTBC, so changing status to that. There was a duplicated issue opened 🐛 Method isClonable throws Plugin not found exception when target_type is not defined in field settings Active which reports the same issue and implements similar solution, but this issue has more steps to reproduce and general activity and is also older.
Did test manually by adding the Dynamic Entity Reference module where I can also confirm the issue exists by showing the Plugin not found exception error. After applying the MR, the issue is resolved and the entity can be cloned. From my POV this can be merged.

It seems there is already a fix for this issue in 🐛 EntityCloneCloneableField isClonable fails when target_type setting is missing Active which also has more comments and MR is approved, so closing this as a duplicate.

The tests are failing because the action is no longer part of core and the module can't be installed. Maintainers need to decide if they still want to support actions by adding a dependency to contributed action module or not and remove test.
Tested manually with a custom entity and can confirm that without the changes in MR the user only sees the clone and cancel buttons. With the MR applied the child entities are shown. If the checkbox is checked, the fields of the referenced entity (created with Base field definition) are cloned.
When cloning the "standard" content types, nothing changes, the cloning works as before, but I did not test with overly complicated entities.
Will leave status to needs review in case tests do get fixed in the meantime and for somebody else to also test and confirm it's working as expected.

This is a very specific case which would need proper steps to reproduce. When creating a custom entity without a bundle, the condition for $field_definition instanceof FieldConfigInterface is usually not met at least not when you want to throw something together to reproduce the issue. When adding a bundle the target_type is usually set. As this code additionally checks that the target_type is set, before loading the entity storage I see no harm in this being added, but will leave the status to needs review if somebody maybe has a custom entity at hand and can reproduce the issue.

Setting status to needs review, did test it and can confirm the issue is present if using content moderation module. The MR solves the issue by forcing the status to publish in case the checkbox is checked. If checkbox is not checked the status is copied from original entity.
The failing tests are due to the action UI module no longer being present in core (from D10.3) and maintainers need to decide if they want to add contributed module dependency or remove action from testing.

The test for action will fail, as the action UI → was removed from core. There is a contributed module that can be used if needed. But that's probably best to handle in separate issue. The commit was reverted to not cause confusion. Will leave the status to needs review so more people can test, but from my POV can be merged.

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

Currently, based on my testing, this actually does not work. The permission is added for node types, but if you check permission for node type, example Article: Clone content for Content editor, it does not actually grant the permission to clone the Article. The Clone all Content entities permission needs to be granted. But that permission grants cloning for all content types.

I tested this with the default Olivero theme and noticed the Request path exclusion always negates the condition. Meaning if I add the Request path exclusion /node/add/* (the Olivero theme needs to be used in FE - the checkbox not ticked under Apperance to use the admin theme when editin or creating content) and then under Reactions add Region -> Disable Regions in Olivero -> Header, the region is actually removed from all other paths, except the /node/add/* path.
The /node/*/edit works the same way and user/*/edit is using the admin theme so no effect there (different regions and display).
I'm not sure if bootstrap is doing something differently, as based on the issue, it just disappears from everywhere, which should not happen. If the issue is still there, could you try with a block as a reaction, just to see if the issue lies with the bootstrap theme?

Could confirm the issue if user would visit a view route that does not exist, like typo or something. If the page is not in maintenance mode, the route is usually 404, meaning it has a name. The MR #57/#58 solves the issue and I think is the correct solution for returning FALSE as it's not a valid route so no need to evaluate it. Marking #57/58 as RTBC.