Currently situation:

  • \Drupal\system\Plugin\views\field\BulkForm is in system module
  • the config schema for that plugin is in Views module
  • the base config schema views_field_bulk_form is in core.data_types.schema.yml

Since all these things depend on the Views module, that sounds like a logical place to put all this, instead of spread all over core.

CommentFileSizeAuthor
#13 2916451-13.patch30.78 KBlendude
#13 interdiff-2916451-7-13.txt9.48 KBlendude
#7 2916451-7.patch21.82 KBlendude
#7 interdiff-2916451-5-7.txt1.02 KBlendude
#5 2916451-5.patch21.74 KBlendude
#2 2916451-2.patch21.75 KBlendude

Comments

Lendude created an issue. See original summary.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
StatusFileSize
new 2916451-2.patch21.75 KB

Lets see what this breaks.....

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: 2916451-2.patch, failed testing. View results

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs work » Needs review
StatusFileSize
new 2916451-5.patch21.74 KB

Bleh that was against 8.4.x

dawehner’s picture

dawehner
Primary language German
commented

We would certainly require a change record for this change, don't you think so?

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
StatusFileSize
new interdiff-2916451-5-7.txt1.02 KB
new 2916451-7.patch21.82 KB

Yeah CR is certainly needed, just didn't expect to come back green the first time :)

Created CR and added the needed 'see' stuff.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Reviewed & tested by the community

Nice catch @Lendude but looking at the patch make me argh! Why did we implement it wrong in the first place? Not blaming anyone or pointing fingers just frustrated about this.
Overall patch looks good change notice also looks good so RTBC.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented

@jibran thanks for looking at it, and I wondered the same thing yesterday, and me and @dawehner came to the conclusion: 'Legacy' :)

Bulk form was part of 'action', which used to live in the system module before the action module was born I believe. So that's why the plugin ended up there to start with.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs review 
  1. +++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
@@ -2,506 +2,20 @@
+@trigger_error(__NAMESPACE__ . '\BulkForm is deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.0. Use \Drupal\views\Plugin\views\field\BulkForm instead. See https://www.drupal.org/node/2916716.', E_USER_DEPRECATED);

    Any particular reason we're using the magic constant here and concatenating rather than just naming the class? 

  2. +++ b/core/modules/views/views.post_update.php
@@ -256,3 +256,10 @@ function views_post_update_entity_link_url() {
+
+/**
+ * Rebuild caches to ensure plugin changes are read in.
+ */
+function views_post_update_bulk_field_moved() {
+  // Empty update to cause a cache rebuild so that the plugin changes are read.
+}

    Don't we need to update config dependencies, not just caches? I mean I guess it's not like system.module could have been disabled nor could Views be, but at the least it would add noise to diffs, no?
     

  3. Looks like we've handled the plugin ID by changing the deprecated one's ID rather than Views', so that part doesn't need an upgrade path. However, might legacy_bulk_form be a more explicit name?
dawehner’s picture

dawehner
Primary language German
commented

Don't we need to update config dependencies, not just caches? I mean I guess it's not like system.module could have been disabled nor could Views be, but at the least it would add noise to diffs, no?

Nice catch! Yeah I guess we would have to save all those views again ...

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Needs work
lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2916451-7-13.txt9.48 KB
new 2916451-13.patch30.78 KB

@xjm thanks for the review!

#10.1 There are instances in core with __NAMESPACE__ and with full namespaces, I like the __NAMESPACE__ pattern personally because it instantly tells you 'this class' instead of having to compare two very similar full namespaces.

#10.2 Nice catch! Updated the update hook and added a test for it.

#10.3 yeah that name is much clearer, changed.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Reviewed & tested by the community

WoW, great catch @xjm. Upgrade path and tests look good.

catch’s picture

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

Committed c203575 and pushed to 8.5.x. Thanks!

  • catch committed c203575 on 8.5.x 
    Issue #2916451 by Lendude, xjm: Move everything related to Bulk Form to...
jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

Thanks, published the change record as well.

Status: Fixed » Closed (fixed)

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