In the filter annotiation class an instance of public $module was missed in #2125045: Don't use module on filters anymore.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug
Unfrozen changes Unfrozen because it only changes a class property to be accurate, and related documentation.
CommentFileSizeAuthor
#21 module_instead_of_provider-2196977-21.patch1.8 KBtadityar
#18 interdiff.txt763 bytestadityar
#17 module_instead_of_provider-2196977-17.patch1.84 KBtadityar
#15 module_instead_of_provider-2196977-15_1.patch1.84 KBtadityar
#8 module_instead_of_provider-2196977-8.patch1.84 KBtadityar
#4 module_instead_of_provider-2196977-4.patch1.87 KBPoornima3
#1 filter-plugin-remaining-references-to-module-2196977-1.patch1.98 KBeric_a

Comments

eric_a’s picture

eric_a commented
Status: Active » Needs review
StatusFileSize
new filter-plugin-remaining-references-to-module-2196977-1.patch1.98 KB
eric_a’s picture

eric_a commented
Title: Remaining references to $module in filter module » Drupal/filter/Annotation/Filter uses public $module instead of $provider
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Needs review » Needs work
Issue tags: +Needs reroll
Poornima3’s picture

Poornima3 commented
StatusFileSize
new module_instead_of_provider-2196977-4.patch1.87 KB

Instead of module providing the filter its now to provider that owns the filter

tadityar’s picture

tadityar commented
Status: Needs work » Needs review

Status change so that the submitted patch could be tested.

tadityar’s picture

tadityar commented
Issue tags: -Needs reroll
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented 
  1. +++ b/core/lib/Drupal/Core/Condition/Annotation/Condition.php
@@ -46,11 +46,13 @@ class Condition extends Plugin {
+   * ¶

    Trailing whitespace here. 

  2. +++ b/core/lib/Drupal/Core/Condition/Annotation/Condition.php
@@ -46,11 +46,13 @@ class Condition extends Plugin {
+  // public $module;

    No need to leave this in place commented out.

tadityar’s picture

tadityar commented
StatusFileSize
new module_instead_of_provider-2196977-8.patch1.84 KB

Corrected the patch.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

This looks good to go!

I added a beta evaluation to the summary.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed 
+++ b/core/modules/filter/src/Entity/FilterFormat.php
@@ -99,7 +99,7 @@ class FilterFormat extends ConfigEntityBase implements FilterFormatInterface, En
-   * - module: The name of the module providing the filter.
+   * - module: The name of the provider that owns the filter.

Oops, looks like we missed a spot. :) (the name of the key) Fixed on commit.

Since this is just a bug fix and not an API break, agreed that this is good to go post-beta.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 1621ae8 on 8.0.x 
    Issue #2196977 by Eric_A, tadityar, Poornima3, jhedstrom: Drupal/filter/...

  • webchick committed e15ebed on 8.0.x 
    Revert "Issue #2196977 by Eric_A, tadityar, Poornima3, jhedstrom: Drupal...
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Fixed » Reviewed & tested by the community

Oops, my bad! I forgot we are in a criticals/majors only freeze atm because there's a beta due out within the next few hours. (See https://groups.drupal.org/node/450568)

Temporarily rolled this back, but I'll commit it again on Thursday if someone else doesn't beat me to it.

In the meantime, if anyone wants to adjust the patch to fix the 'module:' key to be 'provider:' instead, go for it. :)

almaudoh’s picture

almaudoh commented 
+++ b/core/lib/Drupal/Core/Condition/Annotation/Condition.php
@@ -46,11 +46,11 @@ class Condition extends Plugin {
+   * The name of the provider that owns the filter.

This description is too specific for the Condition annotation. Something more like "The name of the provider of the condition."

tadityar’s picture

tadityar commented
StatusFileSize
new module_instead_of_provider-2196977-15_1.patch1.84 KB

fixed the name of the key :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: module_instead_of_provider-2196977-15_1.patch, failed testing.

tadityar’s picture

tadityar commented
Status: Needs work » Needs review
StatusFileSize
new module_instead_of_provider-2196977-17.patch1.84 KB

That was pretty weird.. I'm sure that I only changed '- module:' to '- provider:' but it failed?
Trying once more.

tadityar’s picture

tadityar commented
StatusFileSize
new interdiff.txt763 bytes

added interdiff between #8 and #17

cs_shadow’s picture

cs_shadow commented
Status: Needs review » Reviewed & tested by the community

RTBC as per #10.

eric_a’s picture

eric_a commented
Status: Reviewed & tested by the community » Needs work

#14 wasn't addressed. But more importantly: why did #4 introduce this Condition class at all in this filter plugin issue?

tadityar’s picture

tadityar commented
Status: Needs work » Needs review
StatusFileSize
new module_instead_of_provider-2196977-21.patch1.8 KB

@Eric_A Ah yes.. Have just noticed #4 edited the wrong file, even the wrong module. Gonna change that, but now that we've figured it shouldn't be inside the Condition class I don't think #14 is necessary. Docs in the rest of the file is also specific (mentioning 'filter').

cilefen’s picture

cilefen commented
Status: Needs review » Reviewed & tested by the community

Now that this is fixing the intended bug, I see no problems with it.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Awesome, thanks for all the attention to detail here. :)

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 233a60d on 8.0.x 
    Issue #2196977 by tadityar, Eric_A, Poornima3, jhedstrom: Drupal/filter/...

Status: Fixed » Closed (fixed)

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