Spin-off from #1626584: Combine configuration system changes to verify they are compatible

1) Cache\DatabaseBackend depends on procedural functions in includes/database.inc.

2) ::checksumTags() throws fatal error if database is not available.

Files: 
CommentFileSizeAuthor
#24 1730774-24-untangle-cache.patch5.24 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 40,420 pass(es). View
#6 1730774-6-untangle-cache.patch6.21 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#4 1730774-4-untangle-cache.patch6.16 KBbeejeebus
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
next.cache-db.0.patch5.58 KBsun
PASSED: [[SimpleTest]]: [MySQL] 39,976 pass(es). View

Comments

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Looks simple enough.

catch’s picture

Why getting the connection each time instead of having it as a class property?

webchick’s picture

Status: Reviewed & tested by the community » Needs review
beejeebus’s picture

FileSize
6.16 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

hmmm. so here's a patch that adds a dbConnection class property, and sets it in the constructor.

feels wrong. should we be using the DIC? how do we do that with different calls per cache backend implementation?

putting this up as a discussion point.

Status: Needs review » Needs work

The last submitted patch, 1730774-4-untangle-cache.patch, failed testing.

beejeebus’s picture

Status: Needs work » Needs review
FileSize
6.21 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

sooooo, Database::getConnection() can get called early during install. reaaaaal early, before it's safe.

Status: Needs review » Needs work

The last submitted patch, 1730774-6-untangle-cache.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

I don't think that a static is the way to go, because the Database connections might change "under the hood." The Database system knows when connection declarations are changing and otherwise quickly returns the appropriate connection already, so duplicating that static information within the DatabaseBackend appears wrong to me.

In turn, I think that #0 is still RTBC.

beejeebus’s picture

there is no static in #6? there are a bunch of non-injectable static calls to Database::getConnection() in #0, but i'm guessing you don't mind those?

colour me confused.

catch’s picture

Assigned: sun » Crell

So the issue to me looks like:

- we want to use dependency injection if possible.
- the cache instance is a 'global' class - i.e. we only want one per request per bin except for very rare exceptions.
- the database connection is also global and can be changed (outside of the DIC)
- we don't want to instantiate a new cache class each call just in case the database connection changed.

To me it feels like #0 is RTBC, but that we might want to eventually jiggle around so that the connection can be injected via the DIC later on (or inject the DIC as a service locator so it can always return the correct connection).

Assigning to Crell since there's going to be several places we need to do similar with the database connection.

beejeebus’s picture

Status: Needs review » Reviewed & tested by the community

meh, i've no idea anymore what is a code smell and what isn't.

but, i suppose Database::getConnection() is no more smelly than db_query(), so i'm probably guilty of scope creep. #0 it is.

Crell’s picture

Status: Reviewed & tested by the community » Needs work

Sorry, these are all wrong. :-) The answer is to put the DB connection into the DIC, and then make the cache object pull it from there when it too is instantiated from the DIC. The installer then needs its own DIC. The Routing patch has the first part done, because I'm doing exactly that. I'll split off the "DB in the DIC" part shortly in a separate patch.

(Note: And then we can move all of our DB-cache tests to unit tests, not integration tests, for better performance and more reliable tests.)

beejeebus’s picture

in #4 i said:

"hmmm. so here's a patch that adds a dbConnection class property, and sets it in the constructor. feels wrong. should we be using the DIC? how do we do that with different calls per cache backend implementation?"

a db connection is just an implementation detail of one cache backend, and many sites won't use that. how do we do that with the DIC?

sun’s picture

Status: Needs work » Needs review

I perfectly understand that there are ideal world implementations and that we'd ideally use the DIC properly, but:

1) we're far away from that and I'm not proposing any architectural change here

2) that's really a completely different issue than fixing the Cache\DatabaseBackend to not rely on procedural code in includes/database.inc and to make it fully take into account that it might be used in situations in which the storage or bin is not available.

Crell’s picture

#1759152: Add a database service to the DIC - The issue, courtesy katbailey.

beejeebus: When using a DIC in this fashion, each implementation would have a different constructor (the constructor is not part of the interface) and the DIC would be wired up accordingly. Eg, cache.page would be wired to the database backend, with the DB connection as a dependency. If you wire cache.page to memcache, you'd instead specify that class with the memcache connection as a dependency. Etc.

beejeebus’s picture

Crell, thanks.

i guess we have a broken interface then, because we have CacheBackendInterface::__construct($bin).

katbailey’s picture

I wasn't aware of this issue and went ahead and created #1762388: Use Dependency Injection for FileStorage, which deals with removing the constructor from the StorageInterface and injecting the DB connection into the DatabaseStorage. So I think it's only a partial solution to what's being discussed here..?

sun’s picture

It's only possible to expose this error with the CachedFileStorage config controller that is being introduced in #1626584: Combine configuration system changes to verify they are compatible for #1702080: Introduce canonical FileStorage + (regular) cache

That said, this issue is about Cache\DatabaseBackend only, not about a config storage controller.

And, you can only expose this error when CachedFileStorage is being used (by default) in the Drupal installer. It is not possible to reproduce the dependency problems on an already installed site.

I also want to stress once more on the fact that the patch in #0

1) merely adjusts the existing Cache\DatabaseBackend to be fully in line with other parts of itself (try/catch for the case a non-existing DB connection or non-existing bin/table)

and in addition to that, it

2) only removes the dependency on the procedural functions in includes/database.inc, which may not be loaded yet.

I'd highly appreciate it if we could move the discussion about properly-using/leveraging the DIC into a separate issue. :)

Crell’s picture

sun: #0 just replaces db_* with Database::getConnection()->*(). That does not reduce the dependencies at all. There's no useful benefit from it. Even if you kept one Database:: call in the constructor and saved that to a property to use later (i.e., #4), that would be a marginal improvement that makes switching to a properly injected connection later a little easier, but #0 doesn't do anything useful at present.

The wacky "everything's broken in the installer" problem is something we should fix, not keep hacking around.

sun’s picture

Erm, the useful benefit is that it unblocks #1702080: Introduce canonical FileStorage + (regular) cache - ?

All I'm asking for to reduce the aims for perfection a bit. We can happily keep this issue open after commit and rework the code entirely to get the connection injected and do whatever else would be nice to have, as long as we can move forward in parallel.

pounard’s picture

I agree with #19, I don't see any benefit in commiting this as-is. It sounds like there is a deeper chicken and egg dependency resolution problem, and it is architectural and lies in the config system, not in the details of the cache backend.

beejeebus’s picture

i've created #1764474: Make Cache interface and backends use the DIC - can we please move the discussion about integrating the cache infrastructure and the DIC over there?

then we can commit this simple fix and unblock #1702080: Introduce canonical FileStorage + (regular) cache

heyrocker’s picture

I agree, sun's fix will get us moving on what is a critical issue for CMI, and we can move larger issues into the followup beejeebus created.

beejeebus’s picture

FileSize
5.24 KB
PASSED: [[SimpleTest]]: [MySQL] 40,420 pass(es). View

here's a reroll of #0 to chase HEAD.

beejeebus’s picture

Status: Needs review » Reviewed & tested by the community

ok, i think this is rtbc. i only rerolled, so it's safe for me to do this.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Even if you kept one Database:: call in the constructor and saved that to a property to use later (i.e., #4), that would be a marginal improvement that makes switching to a properly injected connection later a little easier, but #0 doesn't do anything useful at present.

See #8, we can't rely on the database connection not changing under our feet with a db_set_active() call before a cache()->get(), which is why I pinged Crell for this issue in the first place :)

Since #1764474: Make Cache interface and backends use the DIC is major, I'm OK committing this as an interim measure to unblock CMI, so I've gone ahead and done that.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.