Remove persist flag from services that do not need it

Do you remember the reason why it was done in the first place. Was that related with performance?

Are we using $this->container in tests now? I didn't keep up with where we stand with that ATM. If that's currently correct then this is RTBC.

I don't remember the original discussions in detail, but I think should get @effulgentsia in here, he probably remembers the most about what and why we did this.

The concept of service persisting was introduced to handle the situation of needing a service object to be the same object (not a newly instantiated one) after a call to module_enable() or module_disable() as it was prior to that call. The original use case of that was #1187726-72: Add caching for configuration / rework config object loading (Was: Memory usage and i/o from config objects). I'm not sure what chx means in that comment by "unsynced", but just hypothesizing, perhaps it's related to a situation of having a modified but unsaved $config object around prior to calling module_(en|dis)able(), and then calling ->save() on it resulted in a weird mismatch between the factory object that created it and the new factory object in the container. Possibly similar weirdnesses for config storage services.

I ran into a similar/related issue in #1878512-2: Clean up Simpletest's kernel/container preparation/rebuild logic now that the kernel is responsive to module enabling, where event subscribers added dynamically to $container->get('event_dispatcher') would become lost if the event_dispatcher service got re-instantiated following a module enable/disable.

It's possible that these specific issues either already have or can be fixed in some way other than having a 'persist' facility. Or that we need to make the persist facility less brittle per the issue summary. Not sure yet which direction makes more sense.

If a process really is

needing a service object to be the same object

then it should get the service before the module enable and store it somewhere using hook_module_preinstall - although I really can not see why you'd want to do this.

One example that I ran into is #2205295: Objectify session handler — the SessionHandler needs to be exposed as a service, because we need to manually write and close the session when the HttpKernel terminates, but yet, session handling is a native global state construct of PHP core and the single SessionHandler instance is registered with PHP core when the session is initialized.

A kernel rebuild/reboot would cause a new SessionHandler instance to be instantiated and terminated, which is different to the original that was registered, and in turn, the session would be written + closed more than once per request. Especially in a batched installation of multiple modules, we'd create as many new SessionHandler instances as the # of modules being installed.

A similar problem space exists for stream wrappers, which are also global state construct of PHP core. The current plan foresees to discover/collect and register all custom stream wrappers when the kernel boots, and to unregister them when it shuts down. Thus far, I missed to account for the kernel rebuild case in the proposed concept, so it is possible that we're facing similar requirements there.

In summary, there appear to be good arguments for removing the 'persist' tag from current core services, but I'm not sure whether we really are in a position to drop the concept entirely, because there appear to be discrete edge-cases (native global state constructs in PHP core) that need it.

That said, a viable alternative might be to hard-code the persisting logic for those PHP core edge-cases directly into the kernel rebuild process. (OTOH, I assume such code would look almost identically to the current persist code...)

What about as a separate step, removing persist from the config system only and leave the concept? In a separate issue. I think that's what @alexpott cares about the most?

Seems like there's agreement then.

+++ b/core/core.services.yml
@@ -178,8 +172,6 @@ services:
   container.namespaces:
     class: ArrayObject
     arguments: [ '%container.namespaces%' ]
-    tags:
-      - { name: persist }
   default_plugin_manager:

It also changes this :)

The only reason we made container.namespaces an ArrayObject and a "service" instead of a array/parameter as it was initially was, was to benefit from the persist flag.

Without the persist flag, that's pretty useless :)

Changing it back would require changing all @container.namespaces to %container.namespaces% and changing all the type hints from \Traversable back to array. Not sure if we want to do that...

Also updated the issue title, the summary might need an update too?

Committed/pushed to 8.x, thanks!

Filed #2277481: Remove persist flag from container.namespaces service.

Re #20: It is in fact not necessary to retain the session-manager instance across container rebuilds: #2272987: Do not persist session manager

Filed #2368263: Remove the persist service tag