Problem/Motivation

In #2887142: NodeType source plugin should include comment information Comment variable sources were deprecated, and a layer of code was added to the comment source code as a bc layer for migrations using these old sources. This BC method can be deprecated in Drupal 9 and have it's usages removed in Drupal 10.

It was un-deprecated in #3069055: Undeprecate Drupal\comment\Plugin\migrate\source\d6\Comment::prepareComment()

Proposed resolution

Remove the usage and deprecate the bc layer method for removal in Drupal 10.

Remaining tasks

Do it! just not yet.

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#30 3069260-30.patch4.88 KBquietone
#30 interdiff-26-30.txt2.97 KBquietone
#30 3069260-30.patch4.96 KBquietone
#30 interdiff-26-30.txt581 bytesquietone
#26 3069260-26.patch4.88 KBquietone
#26 interdiff-25-26.txt430 bytesquietone
#25 3069260-25.patch4.89 KBquietone
#25 interdiff-20-25.txt3.09 KBquietone
#24 3069260-23.patch12.17 KBrahulkhandelwal1990
#23 interdiff_22-23.txt1.56 KBrahulkhandelwal1990
#22 interdiff_20-22.txt8.23 KBmeenakshi_j
#22 3069260-22.patch10.22 KBmeenakshi_j
#20 3069260-20.patch1.69 KBandypost
#20 interdiff.txt1.77 KBandypost
#19 3069260-19.patch1.08 KBhmendes
#19 interdiff_18-19.txt787 byteshmendes
#18 3069260-18.patch1.08 KBguilhermevp
#18 interdiff_15-18.txt967 bytesguilhermevp
#14 interdiff_8_11.txt2.4 KBanmolgoyal74
#14 3069260-14.patch1.06 KBanmolgoyal74
#8 3069260-8.patch2.42 KBswatichouhan012
#6 3069260-6.patch2.41 KBandypost
#2 3069260-2.patch2.22 KBmikelutz

Comments

mikelutz created an issue. See original summary.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
StatusFileSize
new 3069260-2.patch2.22 KB
mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Title: Remove Drupal\comment\Plugin\migrate\source\d6\Comment::prepareComment() and it's usage. » Deprecate Drupal\comment\Plugin\migrate\source\d6\Comment::prepareComment().
Issue summary: View changes

Version: 9.x-dev » 9.0.x-dev

The 9.0.x branch will open for development soon, and the placeholder 9.x branch should no longer be used. Only issues that require a new major version should be filed against 9.0.x (for example, removing deprecated code or updating dependency major versions). New developments and disruptive changes that are allowed in a minor version should be filed against 8.9.x, and significant new features will be moved to 9.1.x at committer discretion. For more information see the Allowed changes during the Drupal 8 and 9 release cycles and the Drupal 9.0.0 release plan.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Postponed » Needs review
Related issues: +#2716163: [META] Remove deprecated classes, methods, procedural functions and code paths outside of deprecated modules on the Drupal 9 branch

Patch in related #3092606: Remove comment.module BC layers removes already deprecated core/modules/comment/src/Plugin/migrate/source/d6/CommentVariablePerCommentType.php

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 3069260-6.patch2.41 KB

Reroll

Status: Needs review » Needs work

The last submitted patch, 6: 3069260-6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

swatichouhan012’s picture

swatichouhan012 commented
Issue tags: +VbContribution2020
StatusFileSize
new 3069260-8.patch2.42 KB

Patch #6 failed to applied, i have removed unused use statement and updated patch.

swatichouhan012’s picture

swatichouhan012 commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: 3069260-8.patch, failed testing. View results

xjm’s picture

xjm
she/her
Primary language English
commented
Version: 9.0.x-dev » 9.1.x-dev

I think we need a proper deprecation here instead of ripping the thing out entirely. :) New deprecations are minor-only, and 8.9/9.0 are in beta, so moving this to 9.1.x. Thanks!

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

quietone’s picture

quietone commented
Issue tags: +Novice

As xjm says this needs a deprecation notice.

I think this is suitable for a novice task. The Drupal core deprecation policy has instructions for adding a deprecation message to a method.

anmolgoyal74’s picture

anmolgoyal74 commented
StatusFileSize
new 3069260-14.patch1.06 KB
new interdiff_8_11.txt2.4 KB

I hope this is fine.

anmolgoyal74’s picture

anmolgoyal74 commented
Status: Needs work » Needs review
guilhermevp’s picture

guilhermevp commented

The patch applies cleanly, however, should we use "BC" in the code documentation?

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

guilhermevp’s picture

guilhermevp commented
StatusFileSize
new interdiff_15-18.txt967 bytes
new 3069260-18.patch1.08 KB

Updated documentation removing the abbreviation of backwards compatible since I could not find any other example of it in core and changed the reference to d10 as a future release. Please review.

hmendes’s picture

hmendes commented
StatusFileSize
new interdiff_18-19.txt787 bytes
new 3069260-19.patch1.08 KB

Fixing patch from #18 to follow standard formats.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes
StatusFileSize
new interdiff.txt1.77 KB
new 3069260-20.patch1.69 KB

Filed CR https://www.drupal.org/node/3221964 and added proper deprecation of the method, also removed the usage

Probably it needs legacy test to make sure the deprecation is thrown when the method is used in inherited plugins

Also updated IS

Status: Needs review » Needs work

The last submitted patch, 20: 3069260-20.patch, failed testing. View results

meenakshi_j’s picture

meenakshi_j commented
StatusFileSize
new 3069260-22.patch10.22 KB
new interdiff_20-22.txt8.23 KB

Tried to fix the fails of #20

rahulkhandelwal1990’s picture

rahulkhandelwal1990 commented
Status: Needs work » Needs review
StatusFileSize
new interdiff_22-23.txt1.56 KB

Uploaded a new patch with test case failure resolution

rahulkhandelwal1990’s picture

rahulkhandelwal1990 commented
StatusFileSize
new 3069260-23.patch12.17 KB

Forgot to upload patch in #23, please review it.

quietone’s picture

quietone commented
Issue tags: -Novice
StatusFileSize
new interdiff-20-25.txt3.09 KB
new 3069260-25.patch4.89 KB

@Meenakshi_j and @rahulkhandelwal1990, thanks for working on this. Whenever you make a patch comment on the changes you made and why you made them. See Make the patch.

The latest patch contains changes to tests, /core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php, /core/modules/comment/tests/src/Kernel/Migrate/d6/MigrateCommentTest.php and /core/modules/comment/tests/src/Kernel/Migrate/d6/MigrateCommentTest.php. Changes to these tests are not expected, or desired, when deprecating a method in a source plugin. The test failures point to a problem caused by some of the code removed.

Did some investigating of the history of the changes for \Drupal\comment\Plugin\migrate\source\d6\Comment::prepareRow and prepareComment are not easy to follow but we have ended up with code in prepareComment() that should be in prepareRow(). The language calculation from #2981392: Comment migration corrupts data with multilingual sites needs to be in prepareRow as does the conversion of the 'status' property.

I have made a new patch based off the patch in #20.

quietone’s picture

quietone commented
StatusFileSize
new interdiff-25-26.txt430 bytes
new 3069260-26.patch4.88 KB

Forgot to run commit-code-check.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

@quietone thank you a lot, it looks cleaner!

quietone’s picture

quietone commented

@andypost, Glad you like it, thanks for the prompt review!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/modules/comment/tests/modules/d6_comment_test/d6_comment_test.info.yml
+++ b/core/modules/comment/tests/modules/d6_comment_test/d6_comment_test.info.yml
@@ -0,0 +1,7 @@

@@ -0,0 +1,7 @@
+name: TBA
+type: module
+description: TBA
+package: Testing

These look like placeholders, can it just be 'D6 comment test'?

Also we're only testing the deprecation message here so is it possible to do that with purely a unit test?

quietone’s picture

quietone commented
StatusFileSize
new interdiff-26-30.txt581 bytes
new 3069260-30.patch4.96 KB
new interdiff-26-30.txt2.97 KB
new 3069260-30.patch4.88 KB

Ah, silly oversight.

This patch adds comments to test module .info file and moves the deprecation test to the existing test of the comment source plugin. It is still a Kernel test but it doesn't load the entire fixture.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

Thanks!

+++ b/core/modules/comment/tests/modules/d6_comment_test/d6_comment_test.info.yml
@@ -0,0 +1,7 @@
+version: VERSION

Not sure that needed

  • catch committed a983c28 on 9.3.x 
    Issue #3069260 by quietone, andypost, guilhermevp, anmolgoyal74,...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Thanks for the tidy up. Committed/pushed to 9.3.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

quietone’s picture

quietone commented

Published the CR