Spinoff #1702080: Introduce canonical FileStorage + (regular) cache .There it is claimed that DatabaseStorage is the only backend we have to test the pluggability -- then it should be a test only system.

Comments

sun’s picture

Issue tags: +Configuration system

Quoting myself from #1702080-22: Introduce canonical FileStorage + (regular) cache #22:

I strongly believe that contributed projects will attempt to advance on the ground-work we're laying with the configuration system for D8. Our goal should be to make it easy for people to experiment with alternative solutions, so we can learn and enhance the system for D9.
...
[The goal is to have a system that] essentially allows for mixing and mashing the built-in storage controllers more easily in novel ways.
...
An essential bottom line I want to draw here is that we do not want to force anyone into presuming and using files as the canonical storage. It might very well be that We're All Wrong™ and that some way smarter guys than us will show us how to implement alternative config storage implementations during D8.
...
I cannot stress enough on the topic to keep these things as pluggable as possible.

Quoting myself from #1702080-71: Introduce canonical FileStorage + (regular) cache #71:

- We want the config storage to be swappable/pluggable.
- Removing DatabaseStorage would only leave FileStorage.
- We need it to validate that storage controllers can actually be swapped out.
- We need it to validate that we do not implement features into a (FileStorage) controller that cannot be supported by others.
- We need it to evaluate and validate that errors are handled in a proper way. (which is not the case yet and an entire issue of its own)

The DatabaseStorage code is already there and done. It is essential for validating and testing the architecture. After this very issue here, we're pretty much done with architectural changes to the config system (except of aforementioned error handling).

The reasoning for removing it is... what exactly? ;) I strongly object to removing it (neither now nor later), because of the reasons outlined above. It's the proof of concept for me — so if anyone comes and says that we said it was possible but it doesn't appear to be the case, then I can point that one to DatabaseStorage and say "look, I've no idea what you want, this alternative implementation here works to 100% and I have proof."

In summary:

It is not only about testing. It is also about pluggability.

The DatabaseStorage controller actually works and is usable. You will agree that it doesn't make sense to use a "testing storage controller" in production.

I haven't heard a single reason other than "meh" for removing it yet. But I'm curious to learn about counter-arguments.

chx’s picture

As for pluggability, aren't we plugging in InstallStorage? And, is the testing of the DIC -- through which we plug in -- is the task of CMI? Also, when testing we plug it in.

Outside usage, what is the real world usage to this? The current FileStorage + CachedStorage will be used 99% percent of the time because the cache is pluggable enough. It'll be an extremely rare case to use something else.

Also the parent issue (your patch, I might say) removed the database table necessary for it and only the test actually makes it working. I am simply asking to finish what was already started -- if it only works with testing why keep it in core/lib?

beejeebus’s picture

i'm not sure what is going on here, whether i'm missing a subtext or something, but i think the reasoning in #1 is really, really off. it seems to boil down to "because we don't know what contrib will do with CMI, we'd better add extra layers of indirection and unused drivers".

CMI will be pluggable via the DIC. all of it. don't like core's implementation? swap it out. we all agree with that.

the core implementation will have a pluggable cache backend. just want to change the cache backend? no problem.

"An essential bottom line I want to draw here is that we do not want to force anyone into presuming and using files as the canonical storage. It might very well be that We're All Wrong™ and that some way smarter guys than us will show us how to implement alternative config storage implementations during D8."

- we all agree that CMI should be pluggable, and with the work we've done / are doing, this will be the case. pluggability has absolutely nothing, at all, to do with wanting to rip out code we're not using. please stop equating those two things

"- We need it to validate that storage controllers can actually be swapped out.
- We need it to validate that we do not implement features into a (FileStorage) controller that cannot be supported by others."

- should we also put a git and bazaar and redis and mongo implementation in core? i mean, just to be safe, right?

"- We need it to evaluate and validate that errors are handled in a proper way. (which is not the case yet and an entire issue of its own)"

- wat? the filestorage code needs to be able to handle errors, as best as we can code it. period. wtf does an unused db storage driver have to do with this?

sun’s picture

The InstallStorage is a fake implementation, as it does not implement all methods (or rather, throws exceptions on most operations).

I'm not fully confident yet that we'll actually keep the active store in FileStorage. We will try it now, for sure, and see how it goes. But one essential part I tried to highlight in #1 is that no one of us can be 100% sure on whether it will actually make sense for D8 production sites. At that point in time, we won't be able to switch it back. But if all goes well with some related DI work, then people will be able to perform a one-line config change to have a sane and working implementation for freshly installed sites. Sure, they'll also have to pre-install the necessary db schema table, but all of that is trivial compared to futzing an early bootstrap dependency into the system.

In any case, we at least need to retain it until we've sorted out the error handling in config storage controllers, since the DatabaseStorage is able to throw vastly different exceptions and errors than the FileStorage... (Starting with rudimentary things like... a file system can never not exist. But ultimately going deep down the rabbit hole of verifying that exceptions/errors in individual methods never get beyond the respective controller, or mayhaps, Config... Anyway, a lot to figure out there, and I want to make sure that we're taking alternate controllers properly into account.)

chx’s picture

Right. I can see the value of keeping it as a test platform. That's all the issue is about.

To clarify, I only mentioned InstallStorage as one way of actually using an alternative backend and while it certainly doesn't write it does test the pluggability. And the suggested move won't change it the least that we test the pluggability with DatabaseStorage. All the issue, let's move it to a test dir, done.

catch’s picture

I don't think we should ship with the DatabaseStorage implementation (outside of tests) unless we're actually going to encourage people to use it. The database table should 100% not be in system_schema() or config_schema()( because that adds overhead to installs (including testing), the table cache in MySQL (imagine 1,000 sites on a shared server, that's 1,000 extra tables) etc. etc.

Crell’s picture

Side note to sun's last point: As of PHP 5.3, it's easy to catch exceptions, wrap them in a new exception, and rethrow a standardized exception. The DB layer is doing that now, and we should probably do the same sort of exception normalization in CMI if we're worried about sometimes getting an SQL error thrown, sometimes getting Redis errors thrown, and sometimes getting a file permission warning raised.

Otherwise I am not following this question close enough to have a strong opinion. Just throwing that out there. :-)

alexpott’s picture

Status: Active » Closed (won't fix)

DatabaseStorage is used for the Config snapshot storage.