The Needs Review Queue Bot β tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide β to find step-by-step guides for working with issues.
- Status changed to RTBC
almost 2 years ago 12:33pm 1 February 2023 - πΊπ¦Ukraine HitchShock Ukraine
The patch really works well for me, +1 RTBC.
Also, found one more issue related to the content syncing, but create a separate issue for that because it's related not only to the migration -
π Skip changing 'revision translation affected' field if the old revision is syncing Needs workAlso, I agree with other people that the patch from π Allow ChangedItem to skip updating the entity's "changed" timestamp when synchronizing Fixed is very useful in this case too.
- @hitchshock opened merge request.
- Status changed to Needs work
almost 2 years ago 3:15pm 9 February 2023 - πΊπΈUnited States benjifisher Boston area
@rclemings: Thanks for updating the issue summary. I am keeping the tag for that because I think it still needs work.
@HitchShock: Thanks for updating the patch (as a MR). When updating an issue that was previously RTBC but needs a "reroll", it is OK to mark the issue RTBC again. Otherwise, it is unusual to mark an issue RTBC when you have worked on the code.
I am setting the status back to NW for several reasons.
-
Is this issue a bug report?
I think there is a pretty good argument that it is. If so, then the issue summary should have steps to reproduce. It looks as though the automated tests implicitly have those steps, but it helps to have them in the issue summary. @quietone already made this point. (See Comments #42, #43.) There may be enough in the existing comments (such as #22 and #35 from @Wim Leers) to make the case that this issue is a bug. See also #11, #55.
If this issue is not a bug report, then we should not change the behavior unconditionally. We can add an option to the destination plugin to use the new behavior. I see that @codebymikey already made this point in #21.
-
Do not change the Entity base class.
This is just a question of OO design. The base class should handle common functionality, and the child classes should handle more specific requirements. In this case,
EntityContentBase::rollback()
can in-line the parent method (2 or 3 lines) instead of modifying the parent method.I see from #36 to #38 that this has already been discussed, and it is more complicated than that. Still, I wonder if there is a better way.
-
Maybe I am reading too much into the phrase, βsimply flags an entity as "syncing"β in the issue description. It suggests to me that this issue will not have any effect itself, and follow-up issues will use that flag. @Berdir knows the effects of
setSyncing()
, but for the rest of us it would help to have an explanation in the issue summary.
-
- π¨π¦Canada smulvih2 Canada π
Patch #47 works for me on drupal/core 9.3.22
- last update
over 1 year ago 29,302 pass, 4 fail - last update
over 1 year ago 29,304 pass - Status changed to RTBC
over 1 year ago 10:55am 22 April 2023 - πΊπ¦Ukraine HitchShock Ukraine
Hi @benjifisher
About your points:- At first glance, this is an extension of the issue
#2985297: Generalize the concept of entity synchronization β
. Only this time we allow marking of migrated entities.
That is, it seems that this is not a bug fix, but the introduction of the new feature. A feature that will allow us to fix π Allow ChangedItem to skip updating the entity's "changed" timestamp when synchronizing Fixed and π Skip changing 'revision translation affected' field if the old revision is syncing Needs work in the future.
But at the same time, this solution automatically solves the issue #3195139: migrate:import --update creates new revisions, if content_moderation active β , or rather, the issue will be fixed by the issues β¨ Mark an entity as 'syncing' during a migration update Needs work and #2803717: Allow 'syncing' content to be updated in content moderation without forcing the creation of a new revision β .
So, in the end, this is not just a new feature, but a new feature + a bug fix.
This is also confirmed by the tests for content_moderation that were created as part of this issue. - As for me, I agree that it's ok to change the base class of the destination plugin. The main goal is to mark all the entities that are migrated, so we need to provide this "flag" to all plugins that extend the base.
- I don't know what exactly you mean, but it's pretty clear to me from the issue description and the related tickets.
Done:
- Added #3195139: migrate:import --update creates new revisions, if content_moderation active β as the related issue and mentioned it in the description.
- Updated tests according to π Move migration tests to the Forum module Fixed
Also, the patch was tested on real projects.
It's RTBC for me.
- At first glance, this is an extension of the issue
#2985297: Generalize the concept of entity synchronization β
. Only this time we allow marking of migrated entities.
- last update
over 1 year ago 29,306 pass - last update
over 1 year ago 29,345 pass - last update
over 1 year ago 29,368 pass - last update
over 1 year ago 29,368 pass - last update
over 1 year ago 29,350 pass, 2 fail - last update
over 1 year ago 29,380 pass - last update
over 1 year ago 29,381 pass - last update
over 1 year ago 29,382 pass - last update
over 1 year ago 29,385 pass - last update
over 1 year ago 29,390 pass - Open on Drupal.org βEnvironment: PHP 8.1 & MySQL 5.7last update
over 1 year ago Waiting for branch to pass - last update
over 1 year ago 29,390 pass - last update
over 1 year ago 29,390 pass - last update
over 1 year ago 29,390 pass - Open on Drupal.org βEnvironment: PHP 8.1 & MySQL 5.7last update
over 1 year ago Waiting for branch to pass 48:46 43:00 Running- last update
over 1 year ago 29,394 pass, 2 fail - last update
over 1 year ago 29,401 pass - last update
over 1 year ago 29,402 pass - last update
over 1 year ago 29,411 pass - last update
over 1 year ago 29,388 pass, 2 fail - last update
over 1 year ago 29,417 pass - last update
over 1 year ago 29,422 pass - last update
over 1 year ago 29,399 pass, 2 fail 18:45 15:05 Running- last update
over 1 year ago 29,431 pass - π¬π§United Kingdom longwave UK
I've been running this for some time along with the related patches in order to fix various issues with migrations; I think it's about time that this one lands in order to be able to move some of those other issues along - hopefully we can get those into 10.2.0 as well.
Removing the issue summary update tag as it's no longer required. Leaving the followup tag as I think #38/39 intend to discuss doing this for config entities as well?
Committed and pushed 68c1000f770 to 11.x (10.2.x). Thanks!
- Status changed to Fixed
over 1 year ago 4:11pm 15 June 2023 -
longwave β
committed 68c1000f on 11.x
Issue #3052115 by huzooka, HitchShock, floydm, ranjith_kumar_k_u, Sam152...
-
longwave β
committed 68c1000f on 11.x
-
longwave β
committed 412f414d on 11.x
Revert "Issue #3052115 by huzooka, HitchShock, floydm, ranjith_kumar_k_u...
-
longwave β
committed 412f414d on 11.x
- Status changed to Needs work
over 1 year ago 5:24pm 15 June 2023 - π¬π§United Kingdom longwave UK
Reverted, because this broke HEAD: https://www.drupal.org/pift-ci-job/2693233 β
1) Drupal\Tests\migrate_drupal_ui\Functional\d6\Upgrade6TestWithContentModeration::testUpgradeAndIncremental LogicException: Missing bundle entity, entity type node_type, entity id forum. /var/www/html/core/lib/Drupal/Core/Entity/EntityType.php:877 /var/www/html/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php:241 /var/www/html/core/lib/Drupal/Core/Plugin/PluginDependencyTrait.php:71 /var/www/html/core/lib/Drupal/Core/Plugin/PluginDependencyTrait.php:89 /var/www/html/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:381 /var/www/html/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:321 /var/www/html/core/modules/workflows/src/Entity/Workflow.php:114 /var/www/html/core/lib/Drupal/Core/Entity/EntityStorageBase.php:528 /var/www/html/core/lib/Drupal/Core/Entity/EntityStorageBase.php:483 /var/www/html/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:257 /var/www/html/core/lib/Drupal/Core/Entity/EntityBase.php:339 /var/www/html/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:609 /var/www/html/core/modules/migrate_drupal_ui/tests/src/Functional/d6/Upgrade6TestWithContentModeration.php:48 /var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
And similarly in the other new test
1) Drupal\Tests\migrate_drupal_ui\Functional\d7\Upgrade7TestWithContentModeration::testUpgradeAndIncremental LogicException: Missing bundle entity, entity type node_type, entity id forum. /var/www/html/core/lib/Drupal/Core/Entity/EntityType.php:877
- last update
over 1 year ago 29,431 pass - Status changed to RTBC
over 1 year ago 9:20am 16 June 2023 - πΊπ¦Ukraine HitchShock Ukraine
@longwave
Updated MR from 10.1.x and there are no test errors atm. -
longwave β
committed e0cbbca6 on 11.x
Issue #3052115 by huzooka, HitchShock, floydm, ranjith_kumar_k_u, Sam152...
-
longwave β
committed e0cbbca6 on 11.x
- Status changed to Fixed
over 1 year ago 9:49am 16 June 2023 - π¬π§United Kingdom longwave UK
It looks like I accidentally committed the patch instead of the MR! I got the right one this time and ran the tests locally to double check.
Committed and pushed cd1b41fdbf to 11.x (10.2.x). Thanks!
Automatically closed - issue fixed for 2 weeks with no activity.
- Status changed to Needs work
about 1 year ago 4:18am 3 October 2023 - Status changed to Fixed
10 months ago 8:59am 4 March 2024 - π³πΏNew Zealand quietone
I made the follow up β¨ Mark a config entity as 'syncing' during a migration update Active
- πΊπΈUnited States john.oltman
This seems like a meaningful change for sites with ongoing migrations and content moderation enabled. Yet I did not see this mentioned in the release notes for 10.2 (sorry if it is there and I missed it). As a result we lost expected revision records since we updated to 10.2 a few weeks ago, which makes it hard to see what the net change is per migration update. The change is not present in 10.1.
By the way I agree with the change. Just wish it had been called out. The easy fix to go back to how things had been working was a presave hook:
/** * Implements hook_entity_presave(). */ function mymodule_entity_presave(EntityInterface $entity) { if (($entity->getEntityTypeId() == 'node') && $entity->isSyncing() && !$entity->isNew()) { $entity->setNewRevision(TRUE); $entity->isDefaultRevision(TRUE); } }
- πΊπΈUnited States jasonawant New Orleans, USA
I found myself in a similar situation as @john.oltman above.
I was relying on 10.1 behaviors to migrate entity revisions' moderation_state values and (I think) relying on ModerationHandler::onPresave() set publish state accordingly.
@john.oltman, thanks for the solution!
- πΊπΈUnited States jasonawant New Orleans, USA
Hi! I created a change record for the 10.1/10.2 changed behavior. Is this needed? If so, could someone review it and then I'll publish it?
Automatically closed - issue fixed for 2 weeks with no activity.