View's ResultRow uses deprecated dynamic properties

as there's no fixed list of properties which could be accepted the workaround looks better

+++ b/core/modules/views/src/ResultRow.php
@@ -5,6 +5,7 @@
+#[\AllowDynamicProperties]

should be enough

Filed coder's issue to allow skip attributes introduced in newer PHP version #3292573: Allow parse/ignore native attributes

Upstream issue for the attribute is https://github.com/squizlabs/PHP_CodeSniffer/issues/3489

Basically it can't be used on earlier PHP version but upstream should care about it instead of coder

Looks related to https://github.com/phpstan/phpstan/issues/7345

PHP 8.2 alpha3 container added, queued CI run for annotation https://www.drupal.org/pift-ci-job/2420836

phpstan fails https://www.drupal.org/pift-ci-job/2420734 so disabled it for testing purpose

@neclimdul at least the test from IS passed now on 8.3-alpha3 image https://dispatcher.drupalci.org/job/drupal_patches/136695/testReport/Vie...

another attempt to ignore Extension class noise as log file > 500MB

using patched version of phpstan helped (need to wait for 1.8.1 version)

Back review as attribute patch passed

Tests still fail on timeout because we needs newer vfs package

#3299853: Apply #[\AllowDynamicProperties] attribute to base classes to make PHP 8.2 log size sane went in with this change, so this could be repurposed to actually provide a replacement API.

> Twig provides a workaround but it clearly discouraging the behavior and looks like this {% set date = attribute(row, 'entity:node/field_publish_date') %}

Fwiw, that is a search api specific thing, that will never happen with SQL queries/"standard" views. But I guess it can't be changed now, at least search_api would need to provide both the old and a normalized key for BC.

#3299853: Apply #[\AllowDynamicProperties] attribute to base classes to make PHP 8.2 log size sane went in with this change, so this could be repurposed to actually provide a replacement API.

Does that make this change from a bug needed for 8.2 compatiblity to a task about code cleanup?

It's no longer a PHP 8.2 blocker, yes, maybe the initial problem about especially search_api identifiers being very hard to use in twig could still be a bug? But personally I'm fine to change it to change it to a task.

now it needs real-api to remove attribute

Somehow it falls to pass

re-parented to #3299855: [META] Get rid of #[\AllowDynamicProperties] attribute

Looks tests are pass now!
I think we still need BC for contrib and custom code but it could use follow-up for 11.x

Here's a patch to get current baseline for failed tests

Locally all tests passed so checking idea from #25

No it has no effect except newly added test

@neclimdul any idea how to move with it?

let's see this way

Fix one test and ensure NULL processed via array_key_exists()

Fix CS

Seems #34 had some failures.

I'll give a try to this. In fact, this issue would be the major blocker for #3345938: Deprecate support for unused \PDO::FETCH_* modes. There, I'll be trying to remove usage of \PDO::FETCH_CLASS fetch mode. That mode is in fact a bit in contraddiction with the philosophy of PHP 8.2 that started tightening on usage of dynamic properties.

@mondrake please elaborate why you're using stdClass instead of keyed array?

I know that this bade class allows dynamic properties but array memory usage os lowed comparing to dynamic class properties

@andypost just because of this

+++ b/core/modules/views/src/Plugin/views/query/Sql.php
@@ -1530,13 +1530,17 @@ public function execute(ViewExecutable $view) {
-        $result->setFetchMode(\PDO::FETCH_CLASS, 'Drupal\views\ResultRow');
+        $result->setFetchMode(\PDO::FETCH_OBJ);

my motivation here is (#36)

trying to remove usage of \PDO::FETCH_CLASS fetch mode.

To me it doesn't matter array or stdClass - however I see the memory usage difference negligible.

For the record, the RFC says about stdClass and deprecation of dynamic properties

stdClass and __get/__set are not affected by this change.

https://wiki.php.net/rfc/deprecate_dynamic_properties

IMHO, we should postpone implementation of an API to access the row’s column values to a follow up.

Here we’re introducing usage of a storage class and magic methods to avoid dynamic properties, and it’s a full scope on its own. With this done, an API should start from deprecating the use of the magic methods.

Added tests for the magic methods.

+++ b/core/modules/views/src/ResultRow.php
@@ -5,9 +5,13 @@
-#[\AllowDynamicProperties]
 class ResultRow {
 
+  /**
+   * Raw row data.
+   */
+  protected \stdClass $data;

This doesn't really feel like much of a win. It feels like we're just doing this because we have `#[\AllowDynamicProperties]`. We're still ending up with a class with dynamic properties. Why not make ResultRow extend from \stdClass? And then I think we can drop the `#[\AllowDynamicProperties]` and have no other new code?

I would definitely prefer making data an array and I don't see a problem with having arbitrary keys in there. That's by design and common, content entities also have an internal $values array property. A property typed on stdClass feels weird to me.

#47

Why not make ResultRow extend from \stdClass?

1. that would not be sufficient to get rid of the #[\AllowDynamicProperties] attribute, see the discussion part in https://wiki.php.net/rfc/deprecate_dynamic_properties#:~:text=Discussion...

2. my motivation is #36, I would like to have a separate storage here, not added properties. No problem to switch back to array per #48.

On this.

FWIW @mondrake extending \stdClass would allow us to remove the attribute - see https://3v4l.org/qalMV#v8.2.3

I wonder about the performance implications of moving to an array and away from dynamic properties on the class. PHP will have double the number of things to track. I.e. instead on 1 object you'll now have an object and an array.

@alexpott thanks for #51 - I then misinterpreted the RFC.

#49.1 does not stand then, edited.

Change to MR workflow. Just #46 applied to the MR.

Changed stdClass to array.

All the code changes look good to me.
The #[\AllowDynamicProperties] has been removed from the class.
Testing has been added.
For me it is RTBC.

This gets rid of the attribute but doesn't really address #2.

I don't quite understand why this adds hasColumn() but no other explicit methods to access the data. either we add get as well or we don't have has either? __isset() exists too? Ah, I I see, it's to replace the property exists check. Unsure about the name too.

I don't think moving to an array from undefined properties is a performance problem, if we're concerned about that then we should probably not rely on magic methods. array_key_exists() is also slower than isset(), if we're concened about performance?

#58 for sure if someone wants to develop a full fledged API according to #2, they may do so. In any case the magic methods will have to stay (even though deprecated) till next major. I think the point is more whether to do it here or in a following issue.

My motivation here is #36, I will not pursue any of the above further.

We're changing from if (property_exists($row, $this->aliases[$column])) { to if ($row->hasColumn($this->aliases[$column])) { - any contrib code that does this will break. As pointed out earlier dynamic properties on \stdClass are not deprecated so we're not really making a change we have to make here. So looking at contrib we'll break things see https://git.drupalcode.org/search?group_id=2&repository_ref=&scope=blobs... - plus if people are doing it in contrib then it will be done in custom code too.

This came up as a daily BSI target.

IS needs to be updated with the proposed solution.
MR needs to be opened for 11.x and address the open threads from 3588

Thanks!