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.
Comment | File | Size | Author |
---|---|---|---|
#16 | 2987089-16.patch | 1.16 KB | andypost |
Comments
Comment #2
saesa CreditAttribution: saesa at SDOS commentedYou 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.
Comment #3
askibinski CreditAttribution: askibinski as a volunteer and at ezCompany commentedI'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?
Comment #4
askibinski CreditAttribution: askibinski as a volunteer and at iO commentedUgh... typo.
Comment #5
idebr CreditAttribution: idebr at iO commentedComment #6
joachim CreditAttribution: joachim as a volunteer commentedLGTM.
Comment #7
alexpottSerialization :( 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.
Comment #15
andypostComment #16
andypostAddress #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
Comment #17
longwaveAgree the service names will not change, if someone wants to change these classes they can override the services injected into ViewExecutableFactory instead.
Comment #18
alexpottDid 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!