Problem/Motivation
In #2670730: Provide a delete action for each content entity type we want to add a generic entity delete action but this needs the User module's tempstore and we can't have core depend on User module code.
Proposed resolution
Move temp store code to core and deprecate the user code.
Remaining tasks
User interface changes
None
API changes
The services user.private_tempstore
and user.shared_tempstore
are deprecated in favour of tempstore.private
and tempstore.shared
.
The following classes are deprecated:
Drupal\user\PrivateTempStore
in favour of Drupal\Core\TempStore\PrivateTempStore
Drupal\user\PrivateTempStoreFactory
in favour of Drupal\Core\TempStore\PrivateTempStoreFactory
Drupal\user\SharedTempStore
in favour of Drupal\Core\TempStore\SharedTempStore
Drupal\user\SharedTempStoreFactory
in favour of Drupal\Core\TempStore\SharedTempStoreFactory
The Drupal\user\TempStoreException
is aliased to Drupal\Core\TempStore\TempStoreException
when the legacy services are used to provide BC if you are catching that exception.
The container parameter user.tempstore.expire
is removed and if it has been customised it is copied to tempstore.expire
and a deprecation notice triggered.
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#22 | 2935617-22.patch | 85.34 KB | alexpott |
#22 | 15-22-interdiff.txt | 19.55 KB | alexpott |
#15 | 2935617-15.patch | 85.75 KB | alexpott |
#15 | 8-15-interdiff.txt | 848 bytes | alexpott |
#8 | 2935617-7.patch | 85.26 KB | alexpott |
Comments
Comment #2
alexpottOne thing to decide is how to handle the removal of the parameter
user.tempstore.expire
. I think the best way might be a service modifier in the user module that checks to see if it is set - if so trigger a silenced deprecation error and set the core value to it.There are also questions about whether we should alias
\Drupal\user\TempStoreException
toDrupal\Core\TempStore\TempStoreException
this would mean that any code catching it wouldn't have to change.Comment #3
alexpottAlso 5 years ago we had #2008884: Provide an interface for TempStore implementations and we moved it out of core.services in #2100313: Move non-core services out of core.services.yml
Comment #4
alexpottIn discussion with @catch we thought the using
class_alias()
might work very nicely for the exception and preserve BC when using the BC services. Patch also adds deprecation of theuser.tempstore.expire
parameter.Comment #5
alexpottComment #6
chr.fritschThis looks really good.
Found a few minors:
Drupal\KernelTests\Core\TempStore\TempStoreDatabaseTest:
Drupal\Tests\Core\TempStore\PrivateTempStoreTest:
Comment #7
chr.fritschForgot to change the status
Comment #8
alexpottFixed #6 and also noticed
Drupal\KernelTests\Core\TempStore\TempStoreDatabaseTest
was installing the user module - that's not necessary now :)Also added the change record I created - https://www.drupal.org/node/2935639 - to the deprecation notices. The CR needs review too.
Comment #9
alexpottUpdated API changes.
Comment #10
catchGood to see the class_alias() works, this will be our first non-test-framework usage afaik.
This means we can't trigger a deprecation message from the old exception, but we're going to have a subset of API changes from 8.x to 9.x that are documented but not automated, and this was the only idea I could come up with for forwards compatibility.
Comment #11
alexpott@catch also in mitigation to not getting the deprecation message from the exception unless you have code that is throwing that exception outside of the core's usages you'll have a deprecation message from the factory and/or the temp store itself.
Comment #12
alexpottAlso for those reviewing the patch the class_alias is tested by the existing tests - see
\Drupal\Tests\user\Unit\PrivateTempStoreTest::testDeleteWithNoLockAvailable()
and\Drupal\Tests\user\Unit\SharedTempStoreTest::testSetWithNoLockAvailable()
for example.Comment #13
chr.fritschI guess we should add a deprecation comment here as well
CR looks good to me.
Comment #14
chr.fritschForget about #13. I missed that we have now the deprecated key.
Comment #15
alexpottCouple of other minor nits I spotted.
Comment #16
chr.fritschI search in the Drupal codebase and couldn't find any other occurrence of the deprecated classes.
So for me, this is looking really good.
Comment #17
alexpottThis conflicts with #2935610: Remove or deprecate MediaDeleteMultipleConfirmForm and it's route definition is that one gets in first this will need a quick reroll.
Comment #18
BerdirWhat's the plan with those tests. Wouldn't it make more sense to move them too and use the new class? Don't think we copied them?
Comment #19
alexpottre #18
All 3 tests that related to temp stores in the User module have been copied to Core tests and the User module tests are all legacy tests with all the necessary @expectedDeprecation annotations.
Comment #20
larowlanthis is the first instance of a core service being prefixed with `core.`. Are we sure want to introduce that here?
We could instead flip it and make it namespaced to the concept - e.g. `tempstore.private` and `tempstore.shared` instead?
We do similar for other services in core, eg cache, config, entity_type.
Thoughts?
Rest of the patch looks awesome, love the deprecation work.
Comment #21
alexpottThere's no need to start setting a precedent here. Let's go with tempstore.private and tempstore.shared. We made a mistake when we adopted the service container by not prefixing everything with the provider but that is not a mistake we can undo here. I'll change the patch / issue summary / change record.
Comment #22
alexpottUpdated the patch and issue summary.
Comment #23
alexpottUpdated the change record as well.
Comment #24
chr.fritschI checked that all occurrences of core.private_tempstore and core.shared_tempstore were replaced by tempstore.private and tempstore.shared.
Comment #25
alexpottFYI I also updated the container parameter from
core.tempstore.expire
totempstore.expire
for consistency. No core container parameters are prefixed withcore.
either.Comment #26
catchCommitted 601593f and pushed to 8.5.x. Thanks!
Comment #28
tim.plunkettThis seems like a cop-out.
Without #2008884: Provide an interface for TempStore implementations this issue makes a bad situation... the same.
This was an opportunity to suggest people typehint by a real interface. Now the deprecation message is prompting them to do the same old thing.
-1
Comment #29
tim.plunkettGlad to see #2008884: Provide an interface for TempStore implementations is moving again. If it doesn't make it into 8.5.x, I don't see why this should either (since the deprecation message will point people to do the wrong thing for typehints)
Comment #30
alexpott@tim.plunkett I disagree. Adding the interface is hard API break for code, this isn't. I agree with adding the interface in 8.x.x but we can only enforce it in 9.0.0
Comment #31
BerdirGave 8.5.x a try on our internal distribution and ctools is very unhappy about this change:
The service "ctools.serializable.tempstore.factory" has a dependency
on a non-existent parameter "user.tempstore.expire". Did you mean
this: "tempstore.expire"? in
I think we need to better BC on the removal of this. my suggestion would be to keep it instead of removing it and copy the value over if it's not the default.
Follow-up?
Edit: Follow-up it is: #2936435: Better BC for deprecated container parameter user.tempstore.expire
Comment #33
newme154 CreditAttribution: newme154 commentedhow is this fixed? I just downloaded 8.6.1 and still getting the issue. why do I have to perform a patch to the latest version if its fixed? does it not get rolled into the newest version?
Comment #34
Berdirwhat isn't fixed? This was a task, it didn't fix any bugs. And there was an issue with BC but that was fixed a long time ago.
Comment #35
newme154 CreditAttribution: newme154 commentedwhy am I getting the error on a fresh install? The status was set to closed (fixed).
Comment #36
BerdirWhat error? If you have an error message you might want to create a new issue and mention everything you think might be relevant.
Comment #37
newme154 CreditAttribution: newme154 commentedsame error message that's in this thread.
[Sun Sep 23 23:29:00.196701 2018] [php7:notice] [pid 17992:tid 1944] [client ::1:59714] Symfony\\Component\\DependencyInjection\\Exception\\ParameterNotFoundException: The service "ctools.serializable.tempstore.factory" has a dependency on a non-existent parameter "user.tempstore.expire". Did you mean this: "tempstore.expire"? in C:\\xampp\\htdocs\\Drupal8\\vendor\\symfony\\dependency-injection\\ParameterBag\\ParameterBag.php on line 102 #0 C:\\xampp\\htdocs\\Drupal8\\vendor\\symfony\\dependency-injection\\ParameterBag\\ParameterBag.php(219): Symfony\\Component\\DependencyInjection\\ParameterBag\\ParameterBag->get('user.tempstore....')\n#1 C:\\xampp\\htdocs\\Drupal8\\vendor\\symfony\\dependency-injection\\ParameterBag\\ParameterBag.php(189): Symfony\\Component\\DependencyInjection\\ParameterBag\\ParameterBag->resolveString('%user.tempstore...', Array)\n#2 C:\\xampp\\htdocs\\Drupal8\\vendor\\symfony\\dependency-injection\\Compiler\\ResolveParameterPlaceHoldersPass.php(64): Symfony\\Component\\DependencyInjection\\ParameterBag\\ParameterBag->resolveValue('%user.tempstore...')\n#3 C:\\xampp\\htdocs\\Drupal8\\vendor\\symfony\\dependency-injection\\Compiler\\AbstractRecursivePass.php(60): Symfony\\Component\\DependencyInjection\\Compiler\\ResolveParameterPlaceHoldersPass->processValue('%user.tempstore...', false)\n#4 C:\\xampp\\htdocs\\Drupal8\\vendor\\symfony\\dependency-injection\\Compiler\\ResolveParameterPlaceHoldersPass.php(79): Symfony\\Component\\DependencyInjection\\Compiler\\AbstractRecursivePass->processValue(Array, false)\n#5 C:\\xampp\\htdocs\\Drupal8\\vendor\\symfony\\dependency-injection\\Compiler\\AbstractRecursivePass.php(67): Symfony\\Component\\DependencyInjection\\Compiler\\ResolveParameterPlaceHoldersPass->processValue(Array)\n#6 C:\\xampp\\htdocs\\Drupal8\\vendor\\symfony\\dependency-injection\\Compiler\\ResolveParameterPlaceHoldersPass.php(79): Symfony\\Component\\DependencyInjection\\Compiler\\AbstractRecursivePass->processValue(Object(Symfony\\Component\\DependencyInjection\\Definition), true)\n#7 C:\\xampp\\htdocs\\Drupal8\\vendor\\symfony\\dependency-injection\\Compiler\\AbstractRecursivePass.php(60): Symfony\\Component\\DependencyInjection\\Compiler\\ResolveParameterPlaceHoldersPass->processValue(Object(Symfony\\Component\\DependencyInjection\\Definition), true)\n#8 C:\\xampp\\htdocs\\Drupal8\\vendor\\symfony\\dependency-injection\\Compiler\\ResolveParameterPlaceHoldersPass.php(79): Symfony\\Component\\DependencyInjection\\Compiler\\AbstractRecursivePass->processValue(Array, true)\n#9 C:\\xampp\\htdocs\\Drupal8\\vendor\\symfony\\dependency-injection\\Compiler\\AbstractRecursivePass.php(39): Symfony\\Component\\DependencyInjection\\Compiler\\ResolveParameterPlaceHoldersPass->processValue(Array, true)\n#10 C:\\xampp\\htdocs\\Drupal8\\vendor\\symfony\\dependency-injection\\Compiler\\ResolveParameterPlaceHoldersPass.php(43): Symfony\\Component\\DependencyInjection\\Compiler\\AbstractRecursivePass->process(Object(Drupal\\Core\\DependencyInjection\\ContainerBuilder))\n#11 C:\\xampp\\htdocs\\Drupal8\\vendor\\symfony\\dependency-injection\\Compiler\\Compiler.php(141): Symfony\\Component\\DependencyInjection\\Compiler\\ResolveParameterPlaceHoldersPass->process(Object(Drupal\\Core\\DependencyInjection\\ContainerBuilder))\n#12 C:\\xampp\\htdocs\\Drupal8\\vendor\\symfony\\dependency-injection\\ContainerBuilder.php(788): Symfony\\Component\\DependencyInjection\\Compiler\\Compiler->compile(Object(Drupal\\Core\\DependencyInjection\\ContainerBuilder))\n#13 C:\\xampp\\htdocs\\Drupal8\\web\\core\\lib\\Drupal\\Core\\DrupalKernel.php(1318): Symfony\\Component\\DependencyInjection\\ContainerBuilder->compile()\n#14 C:\\xampp\\htdocs\\Drupal8\\web\\core\\lib\\Drupal\\Core\\DrupalKernel.php(892): Drupal\\Core\\DrupalKernel->compileContainer()\n#15 C:\\xampp\\htdocs\\Drupal8\\web\\core\\lib\\Drupal\\Core\\DrupalKernel.php(468): Drupal\\Core\\DrupalKernel->initializeContainer()\n#16 C:\\xampp\\htdocs\\Drupal8\\web\\core\\lib\\Drupal\\Core\\DrupalKernel.php(664): Drupal\\Core\\DrupalKernel->boot()\n#17 C:\\xampp\\htdocs\\Drupal8\\web\\index.php(19): Drupal\\Core\\DrupalKernel->handle(Object(Symfony\\Component\\HttpFoundation\\Request))\n#18 {main}
Comment #38
minhein CreditAttribution: minhein as a volunteer and commentedI am getting same error too.
Symfony\Component\DependencyInjection\Exception\ParameterNotFoundException: The service "ctools.serializable.tempstore.factory" has a dependency on a [error]
non-existent parameter "user.tempstore.expire". Did you mean this: "tempstore.expire"? in
/apps/drupal/htdocs/vendor/symfony/dependency-injection/ParameterBag/ParameterBag.php:102
Comment #39
larowlandid you update ctools module?
Comment #40
ARRC-Drupal-Chick CreditAttribution: ARRC-Drupal-Chick commented#39, I am also experiencing issues related to user temp store and Chaos Tools. I was good until Chaos Tools updated twice this month.
Any attempts to install ctools 8.x-3.1 or 8.x-3.2 (both released this month) causes any Drupal based page to crash once the code has been changed:
This is the message:
TypeError: Argument 1 passed to Drupal\ctools\ParamConverter\TempstoreConverter::__construct() must be an instance of Drupal\user\SharedTempStoreFactory, instance of Drupal\Core\TempStore\SharedTempStoreFactory given in {full_path_removed}\modules\ctools\src\ParamConverter\TempstoreConverter.php on line 102
The fault here appears to be that Chaos Tools updated to use the deprecated user tempstore.
I have a configuration that does not allow the use of Drush (and, yes, it is unusual but has been working in production for 3 years):
Comment #41
BerdirCtools was updated to use the non-deprecated service. If that's the error then IMHO something went very wrong with your update. Make sure you don't have multiple versions of the ctools module lying around in different folder or so and that you updated all files. You might want to remove the folder completely and replace it.
If it's still not working then post in the ctools issue queue, this isn't a problem in core.
Edit: I just saw you commented there too, but as I said, something with your code base is not correct. Note that you can still use drush (or composer) to manage your files on a local/development system, even if you can't use it on your production server.