Problem/Motivation

Modules should not directly use the _serviceId property directly, because it is an internal property which is not always available. The code in ViewExecutable.php will generate PHP notices when this property is unavailable, for example when decorating the core route_provider (#2536370: DependencySerializationTraitPass does not add _serviceId to decorated services).

There seems to be 1 other core module (rest) which also has this issue:
#2935503: Remove rest.module's use of _serviceId for module resolution

Proposed resolution

Replace the direct usage of the _serviceId property in __sleep() (ViewExecutable.php) by retrieving the id from the services.

      'views_data' => $this->viewsData->_serviceId,
      'route_provider' => $this->routeProvider->_serviceId,

Remaining tasks

Write patch. Review. Test.

User interface changes

None.

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

askibinski created an issue. See original summary.

saesa’s picture

You can also see the same problem in /core/tests/Drupal/Tests/Core/DependencyInjection/ContainerTest.php in line 32.
$this->assertEquals('bar', $class->_serviceId);
Field '_serviceId' not found in Fixture\BarClass less. Referenced field is not found in subject class.
We should check is not performed on objects of type "stdClass" or derived.

askibinski’s picture

I'm not sure how to do this, but in this case, since the service id's can't be changed (or can they?) why not just set them?

askibinski’s picture

idebr’s picture

Status: Active » Needs review
joachim’s picture

Status: Needs review » Reviewed & tested by the community

LGTM.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Serialization :( such a tricky thing with services about. If we going to go this way I feel that we should remove views_data and route_provider from the serializationData array and just do the hard coding in __wakeup().

I think we could consider trying to use DependencySerializationTrait here to do the container stuff for us.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

andypost’s picture

Address #7 - there's no reason to serialize this properties at all as service names should not change

So using direct service names in __wakeup()

PS: the bonus is less data to store/load when views are serialized

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Agree the service names will not change, if someone wants to change these classes they can override the services injected into ViewExecutableFactory instead.

alexpott’s picture

Version: 9.5.x-dev » 9.4.x-dev
Status: Reviewed & tested by the community » Fixed

Did we try to use \Drupal\Core\DependencyInjection\DependencySerializationTrait? That's what we use for this everywhere else. I guess the fact we're doing a load of special work in __sleep / __wakeup to limit what we serialize means this approach is okay. Maybe once #2531564: Fix leaky and brittle container serialization solution lands we can reconsider what we are doing.

Committed and pushed c7dc516fdc to 10.1.x and 4ede4db245 to 10.0.x and d83d8eb4df to 9.5.x and c381152e90 to 9.4.x. Thanks!

  • alexpott committed c7dc516 on 10.1.x
    Issue #2987089 by askibinski, andypost, alexpott: Remove views.module's...

  • alexpott committed 4ede4db on 10.0.x
    Issue #2987089 by askibinski, andypost, alexpott: Remove views.module's...

  • alexpott committed d83d8eb on 9.5.x
    Issue #2987089 by askibinski, andypost, alexpott: Remove views.module's...

  • alexpott committed c381152 on 9.4.x
    Issue #2987089 by askibinski, andypost, alexpott: Remove views.module's...

Status: Fixed » Closed (fixed)

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