- CommentVariablePerCommentType and mentions, related #3069260: Deprecate Drupal\comment\Plugin\migrate\source\d6\Comment::prepareComment().
- src/Tests/CommentTestBase.php
- src/Tests/Views/CommentTestBase.php
- tests/src/Kernel/CommentLegacyTest.php
- procedural wrappers
- actions and related config schema
- constants

CommentFileSizeAuthor
#24 3092606-24.patch54.81 KBWim Leers
#24 interdiff.txt6.08 KBWim Leers
#22 3092606-22.patch47.41 KBWim Leers
#22 interdiff.txt743 bytesWim Leers
#20 3092606-20.patch47.43 KBandypost
#16 3092606-16.patch49.16 KBandypost
#16 interdiff.txt2.37 KBandypost
#13 interdiff.txt1.81 KBandypost
#13 3092606-13.patch47.43 KBandypost
#11 3092606-11.patch49.62 KBandypost
#11 interdiff.txt1.94 KBandypost
#8 3092606-8.patch51.54 KBandypost
#8 interdiff.txt3.64 KBandypost
#6 3092606-6.patch54.83 KBandypost
#6 interdiff.txt24.04 KBandypost
#2 3092606-2.patch31.01 KBandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Active » Needs review
FileSize
3092606-2.patch31.01 KB
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Related issues: +#3092615: Clean-up CommentStorage constructor
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Deprecation Removal

👍

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +needs re-roll

Needs a re-roll.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs work » Needs review
Issue tags: -needs re-roll
FileSize
interdiff.txt24.04 KB
3092606-6.patch54.83 KB

There's more removals for 9.0 so looks now all covered

Status: Needs review » Needs work

The last submitted patch, 6: 3092606-6.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs work » Needs review
FileSize
interdiff.txt3.64 KB
3092606-8.patch51.54 KB

Revert removal of comment actions used in upgrade tests

Looks it needs separate issue to fix dump usage

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

@andypost #3087644: Remove Drupal 8 updates up to and including 88** will solve that I think? Should we postpone on that?

Status: Needs review » Needs work

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

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs work » Needs review
Related issues: +#3087644: Remove Drupal 8 updates up to and including 88**
FileSize
interdiff.txt1.94 KB
3092606-11.patch49.62 KB

@longwave thank you! looks it should be blocked on it

Revert one more action

Status: Needs review » Needs work

The last submitted patch, 11: 3092606-11.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs work » Needs review
FileSize
3092606-13.patch47.43 KB
interdiff.txt1.81 KB

so no way to remove actions

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Yes, we should probably wait until the updates are removed, or we'll need another issue for the actions?

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Filed follow-up specifically for actions - #3097204: Remove deprecated action plugins from 9.0.x

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
FileSize
interdiff.txt2.37 KB
3092606-16.patch49.16 KB

Add changes from #3069260: Deprecate Drupal\comment\Plugin\migrate\source\d6\Comment::prepareComment().

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Related issues: +#3039240: Create a way to declare a plugin as deprecated 
+++ /dev/null
@@ -1,103 +0,0 @@
- *   id = "d7_comment_type",

used or not?!

core/modules/comment/migrations/d7_comment.yml:39:      migration: d7_comment_type
core/modules/comment/migrations/d7_comment.yml:61:    - d7_comment_type
core/modules/comment/migrations/d7_comment_entity_display.yml:21:      migration: d7_comment_type
core/modules/comment/migrations/d7_comment_entity_form_display.yml:20:      migration: d7_comment_type
core/modules/comment/migrations/d7_comment_entity_form_display_subject.yml:24:      migration: d7_comment_type
core/modules/comment/migrations/d7_comment_entity_form_display_subject.yml:41:    - d7_comment_type
core/modules/comment/migrations/d7_comment_field.yml:17:      migration: d7_comment_type
core/modules/comment/migrations/d7_comment_field.yml:27:    - d7_comment_type
core/modules/comment/migrations/d7_comment_field_instance.yml:20:      migration: d7_comment_type
core/modules/comment/migrations/d7_comment_type.yml:1:id: d7_comment_type
core/modules/config_translation/migrations/d7_field_instance_label_description_translation.yml:61:    - d7_comment_type
core/modules/config_translation/migrations/d7_field_instance_option_translation.yml:24:#  # d7_comment_type.yml for more information.
core/modules/content_translation/migrations/d7_entity_translation_settings.yml:30:    - d7_comment_type
core/modules/content_translation/migrations/d7_language_content_comment_settings.yml:59:    - d7_comment_type
core/modules/content_translation/tests/src/Kernel/Migrate/d7/MigrateEntityTranslationSettingsTest.php:51:      'd7_comment_type',
core/modules/field/migrations/d7_field_formatter_settings.yml:32:  # d7_comment_type.yml or more information.
core/modules/field/migrations/d7_field_instance.yml:21:  # d7_comment_type.yml for more information.
core/modules/field/migrations/d7_field_instance.yml:61:    - d7_comment_type
core/modules/field/migrations/d7_field_instance_widget_settings.yml:32:  # d7_comment_type.yml for more information.
core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceLabelDescriptionTest.php:54:      'd7_comment_type',
core/modules/migrate_drupal/tests/src/Kernel/d7/MigrateDrupal7TestBase.php:72:    $this->executeMigration('d7_comment_type');
core/modules/rdf/migrations/d7_rdf_mapping.yml:35:    - d7_comment_type

Status: Needs review » Needs work

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

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

probably better to revert last changes as they are not commited

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Issue summary: View changes
Status: Needs work » Needs review
Related issues: +#3097204: Remove deprecated action plugins from 9.0.x
FileSize
3092606-20.patch47.43 KB

Reverted changes from #16 and updated summary with follow-ups

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented 
  1. +++ b/core/modules/comment/src/Plugin/migrate/source/d6/Comment.php
@@ -44,7 +44,7 @@ public function prepareRow(Row $row) {
-   * plugins d6_comment_variable and d6_comment_variable_per_comment_type.
+   * plugins d6_comment_variable.

    "plugin" not "plugins" 

  2. +++ /dev/null
@@ -1,114 +0,0 @@
- *   id = "d6_comment_variable",

    But we are deleting it here anyway, so why still reference it at all?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
interdiff.txt743 bytes
3092606-22.patch47.41 KB

With this patch applied, I still find 8 @deprecated occurrences in core/modules/comment. But all of those are either for @Action plugins or for comment.schema entries for those @Action plugins. Per #3097204 and specifically #3097204-5: Remove deprecated action plugins from 9.0.x, we should not remove @Action plugins in this issue.

#21: Agreed, fixed that. Note that that code already points to #3069260: Deprecate Drupal\comment\Plugin\migrate\source\d6\Comment::prepareComment(). as the issue to remove that code altogether, so I'm not touching the code, only changing the comments as requested.

Other than that, this looks ready to me. 🚢

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

> Per #3097204 and specifically #3097204-5: Remove deprecated action plugins from 9.0.x, we should not remove @Action plugins in this issue.

Except now the upgrade path issue is in, so we could just as well remove them now and the others in the respective module's issue.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Reviewed & tested by the community » Needs review
FileSize
interdiff.txt6.08 KB
3092606-24.patch54.81 KB

#23: Done. Since system_post_update_change_action_plugins() + system_post_update_change_delete_action_plugins() also provide an automatic update path away from the deprecated @Action plugins, I can see how we can consider them properly deprecated.

(Unlike migration plugins, which can have migration definitions referring to migration source/destination/process plugins explicitly, hence making an automatic update path impossible.)

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

Thanks. IMHO, these classes/plugins are deprecated "enough" (we have the @trigger_error() in the constructor) and are safe to remove. The other issue is IMHO more about handling them in the UI, but core doesn't show non-configurable plugins in the UI atm, so that's not a problem. They might have usages in other places like views_bulk_operations but that will have to deal with that.

Alternatively, if someone disagrees, then we can still commit the patch in #22.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

I agree with @Berdir that a plugin with a deprecation in the constructor is fair game for removal.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: 3092606-24.patch, failed testing. View results

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community

Random failure in Drupal\Tests\media_library\FunctionalJavascript\WidgetUploadTest strikes again.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Issue summary: View changes

fixed summary

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed fc46c7f and pushed to 9.0.x. Thanks!

  • alexpott committed fc46c7f on 9.0.x 
    Issue #3092606 by andypost, Wim Leers, Berdir, longwave: Remove comment....

Status: Fixed » Closed (fixed)

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