Problem/Motivation

part of #3299855: [META] Get rid of #[\AllowDynamicProperties] attribute
Remove attribute added in #3299853: Apply #[\AllowDynamicProperties] attribute to base classes to make PHP 8.2 log size sane for PluginBase

Proposed resolution

provide API (probably magic __get()/__set()) to add properties to views plugins dynamically

Remaining tasks

- agree
- patch/review/commit

User interface changes

no

API changes

TBD

Data model changes

no

Release notes snippet

no

CommentFileSizeAuthor
#13 3299858-13.patch2.85 KBandypost
#13 interdiff.txt13.3 KBandypost
#10 3299858-10.patch13.64 KBandypost
#10 interdiff.txt1.36 KBandypost
#9 3299858-9.patch13.59 KBandypost
#6 3299858-6.patch1.63 KBandypost
#9 interdiff.txt9.7 KBandypost
#7 3299858-7.patch4.45 KBandypost
#6 interdiff.txt1.38 KBandypost
#4 3299858-4-baseline.patch532 bytesandypost
#7 interdiff.txt2.82 KBandypost

Comments

andypost created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#3210004: Rss views style plugin channel_elements property needs rename, +#3299859: Remove AllowDynamicProperties attribute from JoinPluginBase, +#3275858: View's ResultRow uses deprecated dynamic properties
andypost’s picture

andypost
he/him
Primary language Russian
commented

Probably magic __get()/__set() could help here but could have override in contrib/custom code

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs review
StatusFileSize
new 3299858-4-baseline.patch532 bytes

let's see baseline

andypost’s picture

andypost
he/him
Primary language Russian
commented
Title: Remove AllowDynamicProperties attribute from PluginBase » Remove AllowDynamicProperties attribute from views/PluginBase
Status: Needs review » Needs work

519 fail

andypost’s picture

andypost
he/him
Primary language Russian
commented
Version: 10.0.x-dev » 10.1.x-dev
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.38 KB
new 3299858-6.patch1.63 KB

As \Drupal\views\ViewExecutable::_preQuery() assigning position property for every views' plugin I moved the definition to PluginBase as the property was added to \Drupal\views\Plugin\views\argument\ArgumentPluginBase::$position in #3295157: Fix 'Access to an undefined property' PHPStan L0 errors - public properties

Patch should fix a lot of tests

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt2.82 KB
new 3299858-7.patch4.45 KB

Checking remains, the $render_tokens which is used only by \Drupal\views\Plugin\views\field\FieldPluginBase so renamed and looking for reviews

 2x: Creation of dynamic property Drupal\views\Plugin\views\style\DefaultStyle::$render_tokens is deprecated
    2x in RevisionRelationshipsTest::testBlockContentRevisionRelationship from Drupal\Tests\block_content\Kernel\Views
andypost’s picture

andypost
he/him
Primary language Russian
commented

And a bit more

  2x: Creation of dynamic property Drupal\views\Plugin\views\argument\StringArgument::$validated_title is deprecated
    1x in ArgumentValidatorTermNameTest::testArgumentValidatorTermName from Drupal\Tests\taxonomy\Kernel\Views
    1x in ArgumentValidatorTermNameTest::testArgumentValidatorTermNameAccess from Drupal\Tests\taxonomy\Kernel\Views

  1x: Creation of dynamic property Drupal\taxonomy\Plugin\views\argument\IndexTid::$validated_title is deprecated
    1x in ArgumentValidatorTermTest::testArgumentValidatorTerm from Drupal\Tests\taxonomy\Kernel\Views

and 

  2x: Creation of dynamic property Drupal\user\Plugin\views\filter\Permissions::$tableAliases is deprecated
    2x in HandlerFilterPermissionTest::testFilterPermission from Drupal\Tests\user\Kernel\Views

1) Drupal\Tests\views\Functional\Plugin\ContextualFiltersStringTest::testUserRoleContextualFilter
Exception: Deprecated function: Creation of dynamic property Drupal\user\Plugin\views\argument\RolesRid::$tableAliases is deprecated
Drupal\views\ManyToOneHelper->ensureMyTable()() (Line: 214)

1) Drupal\Tests\views\Functional\Plugin\ExposedFormCheckboxesTest::testExposedIsAllOfFilter
Exception: Deprecated function: Creation of dynamic property Drupal\taxonomy\Plugin\views\filter\TaxonomyIndexTid::$tableAliases is deprecated
Drupal\views\ManyToOneHelper->ensureMyTable()() (Line: 214)
andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt9.7 KB
new 3299858-9.patch13.59 KB

Attempt to fix, looks better to split every property into own issue

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt1.36 KB
new 3299858-10.patch13.64 KB

It's an array of strings

The last submitted patch, 9: 3299858-9.patch, failed testing. View results

lendude’s picture

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

@andypost asked me to chime in with an opinion, so here it is.

+++ b/core/modules/views/src/Plugin/views/PluginBase.php
@@ -107,6 +106,16 @@ abstract class PluginBase extends ComponentPluginBase implements ContainerFactor
+   * The handler position.
+   *
+   * @see \Drupal\views\ViewExecutable::_preQuery()
+   * @see \Drupal\views\Plugin\views\argument\ArgumentPluginBase::getValue()
+   * @see \Drupal\views\Plugin\views\display\DisplayPluginBase::renderArea()
+   * @see template_preprocess_views_view_summary()

These @see everywhere are very helpful now, but will at some point get hopelessly outdated since we will never ever remember to update them when the code being referred to changes for whatever reason. So do we really want these? No idea how they were done in other issues like this one.

I'm fine with renaming the variables to camelcase, but this could be considered a BC break. This being Views, there is a high possibility that somebody somewhere is making use of these variables under their current name in a way we don't expect.
Will a change record suffice or should we do deprecation errors (which might be tricky)? We need an answer on that.

Setting to needs work to get a documented decision on these questions.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#2052421: [META] Rename Views properties to core standards
StatusFileSize
new interdiff.txt13.3 KB
new 3299858-13.patch2.85 KB

There's some usage in contrib

- http://codcontrib.hank.vps-private.net/search?text=validated_title
- http://codcontrib.hank.vps-private.net/search?text=render_tokens

so I reverted it and tuned docblocks, let's see if sniffers will allow it (I gonna explore DeprecatedServicePropertyTrait meantime)

That's looks minimal patch

let's do rename via #2052421: [META] Rename Views properties to core standards

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
Related issues: -#2052421: [META] Rename Views properties to core standards +#2062177: In ArgumentPluginBase Rename Views properties to core standards, +#3344578: In StylePluginBase Rename Views properties to core standards

Specific follow-ups are

- validated_title existing #2062177: In ArgumentPluginBase Rename Views properties to core standards
- render_tokens filed #3344578: In StylePluginBase Rename Views properties to core standards

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs review » Reviewed & tested by the community

Nice and minimal and the follow ups look good to me.

catch’s picture

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

Committed ab9bb57 and pushed to 10.1.x. Thanks!

  • catch committed ab9bb579 on 10.1.x 
    Issue #3299858 by andypost, Lendude: Remove AllowDynamicProperties...
catch’s picture

catch
he/him
Primary language English
commented

Status: Fixed » Closed (fixed)

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