Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Spin-off from #1320648: Meta: start converting existing core classes to PSR-0 [policy, no patch].
Postponing this on #1272706: Remove backwards compatibility layer for cache API which removes a lot of procedural wrappers we'd otherwise need to figure out where to put somewhere.
Comment | File | Size | Author |
---|---|---|---|
#110 | cache.nodb_.patch | 6.37 KB | effulgentsia |
#106 | cache-backends-legacy.patch | 677 bytes | effulgentsia |
#98 | cache-psr0-98.patch | 3.83 KB | amateescu |
#91 | drupal-1323120-91.patch | 2.97 KB | kotnik |
#90 | missed-a-few.patch | 3.21 KB | David_Rothstein |
Comments
Comment #1
pounard@catch I can work on this issue if you want, I'd be pleased to do it since I'm trying to work on other parts of cache handling. Once the other has been resolved of course.
Comment #2
pounardI know this is postponed, but here is a 20mn work that should definitely work.
Comment #4
pounardOups forgotten added files...
Comment #5
pounardRetrying with the right patch...
Comment #7
pounardThe site, installation and usage works fine with the patch, and the SimpleTest error really is obscure. I guess this is a SimpleTest related error? Whatever at least this patch is fine for everyone that needs to start working on this issue: it's a good place to start.
Comment #8
pounardUpdate patch, I'm not convinced it will pass tests, but at least it works flawlessly on my test site.
Comment #10
pounardSame but removed useless code in install.core.inc, hoping this was the source of problems.
Comment #12
Crell CreditAttribution: Crell commentedCan this be called something more generic? It's not useful only during install. It's useful any time you want Caching to be a no-op. Maybe "NullBackend" or "EmptyBackend"?
I'm also not clear on why this extends the DB backend, but that's in the current code now so we probably shouldn't try to change that in this patch.
24 days to next Drupal core point release.
Comment #13
pounardThe real NullBackend already exists, name was choosen in an older issue (I'm too lazy to find the link). The InstallBackend has been kept intentionnally (I'm not the one that wanted to keep it) because its weird implementation really fix a annoying bug dues to parallel AJAX requests over the incomplete site during install.
Comment #14
aspilicious CreditAttribution: aspilicious commentedtrailing whitespace
23 days to next Drupal core point release.
Comment #15
neclimdulPatch applies cleanly for me so not sure what the test bot's problem is. However... seeing some weirdness. Will report shortly after some investigation.
Comment #16
neclimdulGetting a weird entry in the semaphore table with the name ':'
What I've found out about it so far is that when you delete it from the table and refresh a page on the site, hidden at the bottom of the page is this fatal error:
Fatal error: Uncaught exception 'PDOException' with message 'SQLSTATE[42S02]: Base table or view not found: 1146 Table 'd_d8.cache_' doesn't exist' in /var/www/localhost/htdocs/d8/core/includes/database/database.inc:2132
Stack trace:
#0 /var/www/localhost/htdocs/d8/core/includes/database/database.inc(2132): PDOStatement->execute(Array)
#1 /var/www/localhost/htdocs/d8/core/includes/database/database.inc(664): DatabaseStatementBase->execute(Array, Array)
#2 /var/www/localhost/htdocs/d8/core/includes/database/database.inc(2311): DatabaseConnection->query('SELECT cid, dat...', Array, Array)
#3 /var/www/localhost/htdocs/d8/core/includes/Drupal/Cache/DatabaseBackend.php(42): db_query('SELECT cid, dat...', Array)
#4 /var/www/localhost/htdocs/d8/core/includes/Drupal/Cache/DatabaseBackend.php(26): Drupal\Cache\DatabaseBackend->getMultiple(Array)
#5 /var/www/localhost/htdocs/d8/core/includes/theme.inc(426): Drupal\Cache\DatabaseBackend->get(NULL)
#6 /var/www/localhost/htdocs/d8/core/includes/bootstrap.inc(419): ThemeRegistry->set(N in /var/www/localhost/htdocs/d8/core/includes/database/database.inc on line 2132
This points at the theme registry. The lock suddenly makes sense also because the registry locks on $cid . ':' . $bin in the set function. What doesn't make sense is how set is magically getting called without a cid or bin all of a sudden.
Comment #17
pounardOh yes definitely, that almost sounds like a typo somewhere, or and old bug already there revealed. I will introspect that I'm quite used to the cache API.
Comment #18
aspilicious CreditAttribution: aspilicious commentedA reroll with all the doc changes
Comment #20
aspilicious CreditAttribution: aspilicious commented#18: 1323120-cache-18.patch queued for re-testing.
Comment #22
aspilicious CreditAttribution: aspilicious commentedFixed doc error
Comment #24
Anonymous (not verified) CreditAttribution: Anonymous commented#22: 1323120-cache-21.patch queued for re-testing.
Comment #26
Anonymous (not verified) CreditAttribution: Anonymous commentedattached patch just fixes a type in cache(), which was still referring to DrupalDatabaseCache instead of Drupal\Cache\DatabaseBackend.
Comment #28
Anonymous (not verified) CreditAttribution: Anonymous commentedi can't reproduce the fail the testbot is reporting locally :-(
Comment #29
Anonymous (not verified) CreditAttribution: Anonymous commentedahem, this time with the new class files.
Comment #31
pounardMerf, let me fix it next week and go have a nice new year's eve :) I did almost all the job at first it's my mess :)
EDIT: Why I don't like working a big patch with too many people is that it's becomming hard to review interdiff patches. We should comment the error we see in the issue queue instead and avoid potential duplicated work; some errors we see might not be errors at all.
Comment #32
pounardDrupal\Cache\DatabaseBackend::get()
does not respect the interface function signature, it may return NULL sometime instead of FALSE, this might be a first problem, not the PDO error thought.Comment #33
Anonymous (not verified) CreditAttribution: Anonymous commentedcall_user_func_array --> system_batch_page --> _batch_page --> _batch_do --> _batch_process --> call_user_func_array --> _simpletest_batch_operation --> DrupalTestCase::run --> DrupalWebTestCase::setUp --> file_prepare_directory --> file_stream_wrapper_valid_scheme --> file_stream_wrapper_get_class --> file_get_stream_wrappers --> module_invoke_all --> module_implements --> Drupal\Cache\DatabaseBackend::get --> Drupal\Cache\DatabaseBackend::getMultiple --> db_query
with query: SELECT cid, data, created, expire, serialized FROM {cache_bootstrap} WHERE cid IN (:cids)
kaboom --> SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal8.simpletest376931cache_bootstrap' doesn't exist
even without this patch, at the time we run file_prepare_directory() in DrupalWebTestCase::setUp(), we haven't created any of the child Drupal's tables. so - HowTF did this ever work?
Comment #34
Anonymous (not verified) CreditAttribution: Anonymous commentedsoooo. when in a namespace,
catch (Exception $e)
doesn't work, we needcatch (\Exception $e)
.Comment #35
aspilicious CreditAttribution: aspilicious commentedWTF...
Now we have \Exception in the prefix stuff and Exception in the clear function o_O.
And why do we need the backslash exactly?
Isn't there any other wat to fix this?
Comment #36
catchShould be a use statement at the top of the file per coding standards.
Comment #37
pounardYes, catch is right.
@beejeebus Well done! Nice catch this one was a really bad typo error.
EDIT: @aspilicious That's normal, because the code is inside a namespace, global namespace (it's not really global though, it's more like the root namespace) is not accessible from here, we have to either use a fully qualified name either use a "use" statement. The opposite would be confusing.
Comment #38
aspilicious CreditAttribution: aspilicious commentedWell that is being discussed again in http://drupal.org/node/1323082
But I'm going to use "use" anyway for now. \Exception is so ugly imo...
Catch do you want this patch or the cache tag patch to be in first?
Hopefully this one is green...
Comment #39
aspilicious CreditAttribution: aspilicious commentedWhoops newline problem
Comment #40
Anonymous (not verified) CreditAttribution: Anonymous commentedawesome, i think use Exception is much better.
sooo - i think this is RTBC, it's mostly just moving / renaming.
Comment #41
pounardOk I was wrong in #31 this is nicely done.
I think it's RTBC too.We should fix #32 first.reset() return FALSE on empty array; so it's definitely OK, RTBC !Comment #42
catchCould we maybe make this CacheBackend instead of BackendInterface. I'm not sure what the standards are in terms of naming stuff like this. If the answer is to keep it as is, that's fine. Since we have DatabaseBackend and InstallBackend classes implementing this it'd match those better, but not sure.
Don't we need to update these comments? I see similar comments updated elsewhere.
Double quotes should work here instead of the escaping?
Comment #43
pounardIt could:
I am not sure myself, I'd say there is none about naming. We should ask Crell maybe.
\ is the escape character for both strings with " and '. It sometimes works without doubling the \, sometime not, depending on the following character. By doubling it, we ensure it always works in all cases.
Yes we definitely need, doing it!.
Comment #44
pounardI'd definitely remove the @file docblock in files, they are redundant with the class doxygen docblock. I'm sorry but I guess that at some point, Dreditor will have to adapt and stop yelling when seeing a file with no such docblock.
Anyone against that?
Comment #45
catchIf we do that we'll need an issue to update the coding standards, it does seem redundant with class per file.
Comment #46
pounardNew patch attached, from #38:
BackendInterface
asCacheBackendInterface
Implements ...
commentsComment #47
pounard@catch As far as I know, there is no PSR-0 code that has been written with those @file docblocks. I know that none has been commited yet, but none has been written with, see for example #1321540: Convert DBTNG to namespaces; separate Drupal bits.
Comment #48
pounard@#42 http://drupal.org/node/608152 states that Interfaces should always have the suffix "Interface".
Comment #49
aspilicious CreditAttribution: aspilicious commentedEvery file needs an @file statement even small css files. You can not remove it.
Comment #50
pounard@aspilicious I do things when then have a goal. I won't add those @file myself if they do nothing else than repeating what's said on the class PHPdoc, even if it's against the rules. As I said, we need Crell's opinion, every bit of PSR-0 code written for core so far DO NOT have the @file.
Comment #51
catchAs I said in #45, we should open a new issue for the coding standards change.
Comment #52
pounard@catch Thanks. As you are official maintainer, I let you the final word about do we need to put those back or not for potential commit. I'd like to see this RTBC quickly so it wouldn't potentially block any further cache API improvement and patches.
Comment #53
catchOK I've opened #1392754: Comply with new documentation standards for @file for namespaced class files.
The explanation given at http://drupal.org/node/1354/#files is that this is used to populate the descriptions at http://api.drupal.org/api/drupal/files.
That's fairly minor but I can see an argument for documenting every file like this even if there's some redundancy.
Comment #54
pounardAdded back @file docblocks as per current standard (I hate this standard). We'll see how it goes later.
Needs review for a potential RTBC state.
Comment #55
pounardAfter aspilicious review, I fixed multiple problems he spotted:
Comment #57
pounardWhat?!
Comment #58
pounardOh my... error! Fixing it right now.
Comment #59
pounardAlways forgot to increment the line numbers when manually editing a patch file, fixed^^
Comment #60
pounardYay! RTBC?
Comment #61
jbrown CreditAttribution: jbrown commentedIt isn't necessary to escape backslashes. It looks really terrible.
can be
It is only necessary to escape a backslash in single quotes when it is the last character in the string or if there are multiple backslashes in a row.
Comment #62
pounard\ is the escape character, even if PHP treats it as a normal character when there's only one, it stills looks messy not escaping it.
From http://www.php.net/manual/en/language.types.string.php#language.types.st... :
Which means that, even if it works without escaping the \ character, it seems more like a side effect behavior due to the fact there is few characters interpreted in single quoted string. In that regard, I'd prefer to do as the PHP official documentation says and double it.
It's a point of view, I'm used to write escaped strings, and seeing a non escaped \ seems terrible to me.
EDIT: As said there: http://drupal.org/node/1353118 using \\ is the standard.
Comment #63
Crell CreditAttribution: Crell commentedHm, my ears are burning... :-)
1) For the DB reshuffling I've been using Drupal\Foo\FooInterface, so CacheInterface or CacheBackendInterface would be consistent interface names. Either is fine. Then implementations of it do NOT need the Cache prefix, since that's implied by the namespace itself. That is, #59 is doing what I would recommend.
2) I'll chime in on the @file issue separately.
3) I would also favor always using the \\ in a string. That way we're consistent, and we don't need to worry about having to do it differently depending on the different quote mechanism, or godforbid if someone changes the quotes and doesn't think to change the backslashes. In the DB reorg I'm using "" in several places so that I can build the full class name dynamically, because it makes the code more readable. It's one less thing I have to think about and can just rely on always working.
Aside from the @file question, at a quick glance this passes my pedantry sniff tests. :-)
Comment #64
pounardThanks :)
Comment #65
catchAll the feedback I had has been fixed or clarified, and seems other people are happy too, so I'm putting this back to RTBC. Will commit in a day or so if nothing else comes up.
Comment #66
jbrown CreditAttribution: jbrown commentedBefore this goes in we would want to be completely sure that we want all namespace components to be in camel case. This patch creates the directory core/includes/Drupal . If we later change to a lowercase 'drupal' then the directory would have to be renamed.
The are various discussions going on about having module canonical machine names (lowercase) as components in namespaces:
It wouldn't make sense for 'Drupal' to be in camel case if it were to be followed by lowercase namespace components.
I think the only component in a namespace that should be in camel case is one that corresponds to a class name. This has the added advantage of not creating sub-directories with capital letters.
Comment #67
David_Rothstein CreditAttribution: David_Rothstein commentedIt looks to me like the code which this patch adds to the new files is not the same code being removed from the original ones? For example:
I do not see these lines removed anywhere in the patch. I'm guessing they were already removed from core, and the patch is accidentally adding them back because it's working from an old version of the codebase?
Also:
There are some grammatical mistakes here (e.g. "class future"). Also, I don't think it's quite accurate; the caches being built during installation aren't "incomplete", but rather they can get out-of-date (because Drupal is unfortunately talking to two different cache systems in alternating page requests, as a result of AJAX; thus if the first didn't clear the second when a cache-clearing event happens, the second would get out of date). It's also a bit optimistic to say this will be dropped; there's an issue for it, but it's a difficult issue, as I recall :)
Can we maybe leave most of this out, and just have a single sentence here? This appears to all be new text added by the patch, but really the patch should just be moving things around rather than adding new documentation, and it seems to me like too much technical detail for the class description; there are already code comments in the clear() method that explain it in more detail.
Speaking of which, and not related to this patch but reviewing it caused me to notice.... #1394648: The installer's cache backend no longer overrides all cache-clearing methods, which can lead to fatal errors
Comment #68
pounard@#66 There is really few chances it will be lower case.
@#67 A lot of revelant notices! Indeed, the patch may have cruft due to other patches that could have been commited since, I need to verify that.
Regarding the installation backend, as long as AJAX request will be made over the incomplete Drupal site being installed during install, this backend will exist. I wanted really badly to remove it, we figured out really weird behaviors.
EDIT: Yes you're right, clear() must disappear.
Comment #69
pounardThis patch is OK as the actual state of core priority between this patch and #1272706: Remove backwards compatibility layer for cache API, which one are we going to commit first?
Thanks a lot to David_Rothstein for raising this point.
Comment #70
pounardThe other one has been commited, simply this one doesn't create any conflicts because it creates new files. Will fix this, thanks a lot David_Rothstein, I almost pushed a regression there.
Comment #71
pounardI diff'ed and fixed the patch on my side, running tests locally then I will send it later in the afternoon.
Comment #72
pounardHere is a new patch attached.
clear()
method cruft from all backendsInstallBackend
to fit with the newdelete*()
methodsInstallBackend::clear()
method code documentation onto the class docblockAcid critiscism and sharp eyes are needed!
Comment #73
pounardComment #74
aspilicious CreditAttribution: aspilicious commentedThis isn't a function anymore so it needs a rewrite. Else it is confusing.
-4 days to next Drupal core point release.
Comment #75
pounardTrue, let's see if the patch passes tests and I will fix it.
Comment #76
pounardFixed the docblock as this:
Changed first sentence and the one aspilicious showed.
Review time!
Comment #77
Crell CreditAttribution: Crell commented#66 All of the other namespace-conversion patches floating around are using Drupal\Subsystem\Classname, with that capitalization. We always capitalize Drupal, and class names must be capitalized, as must their files per PSR-0. So module name is really the only odd-man-out, and we can deal with that later.
Also, http://drupal.org/node/1354#files was just updated tonight with how to use the @file directive for single-class files per #1392754: Comply with new documentation standards for @file for namespaced class files. Sorry, pounard. :-)
Comment #78
aspilicious CreditAttribution: aspilicious commentedI can do this
Comment #79
pounardGood, thanks!
Comment #80
sunComment #81
catchOK I read through again, the regression of the other patch is completely removed now, and everything else looks good.
Third time lucky?
Comment #82
sunRaised two questions on this patch over in #1320648-22: Meta: start converting existing core classes to PSR-0 [policy, no patch], as they apply to all PSR-0 patches.
Comment #83
pounardAside of those questions, which are not blocking anyway, I think the patch is good to go. We need to do a documentation effort in the cache backend interface, I raised this point in one of the Redis module issues, but this is a different matter I may open a separate core issue for that soon.
Comment #84
aspilicious CreditAttribution: aspilicious commentedJust converted the double backslash to a single backslash as discussed in the meta issue.
Comment #85
catchThanks! Committed/pushed to 8.x.
We should probably add a change notification for the renaming of the null and database cache backends, since people will have to update their settings.php if they explicitly reference those. I think that's the only thing in the patch that needs a notification though.
Comment #86
Crell CreditAttribution: Crell commentedCongratulations pounard for getting the first Drupal\ namespaced code in Drupal. :-)
Comment #87
webchickFirst; Yay!
Second:
Missed one. :)
Comment #88
pounard@Crell I was not alone, everyone that participated in here ows the merit :)
Comment #89
pounard@catch I might need the "revisit later" tag for the PHP doxygen following the comment http://drupal.org/node/1320648#comment-5464260 ?
Comment #90
David_Rothstein CreditAttribution: David_Rothstein commentedWhile we're doing followups, it looks like a few places in the documentation were not updated for the new class and interface names. I've added fixes for those to @webchick's patch.
Comment #91
kotnik CreditAttribution: kotnik commentedDue to changes in #1400748: Proposal for unified namespace organization here's fixed patch witch changes from #87, #90 and one more.
Comment #93
xjmThe current task here isn't writing the change notification; please re-tag etc. when the code is settled. :)
Comment #94
catch#91: drupal-1323120-91.patch queued for re-testing.
Comment #95
neclimdul@kotnik is there a reason that the install fix disappeared from your patch?
Comment #96
kotnik CreditAttribution: kotnik commented@neclimdul, could you, please, write what install fix do you mean?
Comment #97
catch@kotnik: see webchick's patch from #87.
Comment #98
amateescu CreditAttribution: amateescu commented@neclimdul, @catch, webchick's change has already been included in http://drupalcode.org/project/drupal.git/commit/037faa8, but this issue still needs some minor @file docblock fixes.
RTBC-ing #90 and #91, my changes are insignificant :)
Comment #99
catchOK committed/pushed this one to 8.x, I think we're back at change notice now.
Comment #100
cosmicdreams CreditAttribution: cosmicdreams commentedIf all this needs is a change notice should this issue be critical?
Comment #101
aspilicious CreditAttribution: aspilicious commentedYes, we make them critical so people actually write them :)
Comment #102
webchickCourtesy of core office hours! http://drupal.org/node/1479568
Comment #104
xjmLovely change notification. Yay!
I stole Crell's example from #1321540: Convert DBTNG to namespaces; separate Drupal bits and added that here as well (along with a note about using a namespace).
Comment #106
effulgentsia CreditAttribution: effulgentsia commentedCan we clean up this straggler? No need for the include files if we're PSR-0, right? Found this as part of #1497366: Introduce Plugin System to Core where we're trying to rename $conf['cache_classes'] to $conf['cache_backends'].
Comment #107
pounardAgree, seems a good patch. I'm not against keeping those lines, but removing them is good either.
IMHO RTBC, please someone else confirm this won't break anyting.
Comment #108
BerdirHm.
This snippet existed because it was the only way to load cache implementation early enough. The registry didn't work for that, nor does the default configuration of the PSR-0 class loader, as modules are loaded only much later.
Right now, as I understand it, you'd need to add a hardcoded PSR-0 configuration to the classloader in settings.php to replace this settings. I think we need to verify that that really works, how exactly it needs to be done (on the module level, sub-directory, what happens if the module-level one is re-added?) and document it, either in an existing or a new change notice (http://drupal.org/node/1534630 is related).
Not sure how that exactly changes with the plugin system but I guess not much, given that there still are module-provided classes that need to be loaded early in the bootstrap-phase.
Comment #109
pounardBerdir is right. If we remove those lines, we surely need to ensure that all the need autoloaders are being set before anything else, even before any cache system has been used, this is, I think, the only condition, but an important one, for removing those.
Comment #110
effulgentsia CreditAttribution: effulgentsia commentedThanks. Good catch. In that case, here's a test we should add to prevent #1497366: Introduce Plugin System to Core from breaking this functionality. Once this is committed, we can figure out how we want to deal with it in the plugin issue.
Comment #111
catchI've been thinking about this with the autoload issue, and so far I mainly got to the point of adding a sites/all/drivers folder (or similar) where you put things like memcache, redis integration, alternative database drivers etc, then those could be registered either all at once or stick with the custom settings.php autoloader registration (assuming that actually works).
This would require re-organisation of the contrib projects (the memcache project would need to split in two for example since it provides both 'drivers' and actual modules), but I don't think that's a big deal - there's not that many, and it'd make setup instructions much, much simpler.
Comment #112
effulgentsia CreditAttribution: effulgentsia commentedThat's good feedback. Thanks. I think it still makes sense to get the test in #110 in first, then follow up with something along the lines of #111. Do you agree, or should we skip #110 and continue directly with #111? Note that #110 identifies that our current config() function is already breaking page_cache_without_database in HEAD and includes a fix/workaround for that.
Comment #113
catchHmm. I'm mildly concerned about the simpletest settings.php include but don't have any specific reason why though. and being able to test this stuff is great.
Is there a reason to fix the page_cache_without_database bug here as opposed to #1576322: page_cache_without_database doesn't return cached pages without the database?
Either way I think getting the test coverage in makes complete sense, this is obviously easy to break!
Comment #114
effulgentsia CreditAttribution: effulgentsia commentedI forgot about that other issue (despite having blindly rerolled the last patch on there just last week, lol). Yeah, let's continue there. Moving this one back to fixed as per #102.
@catch: Given your upcoming unavailability, I would very much appreciate if you could review #1497366: Introduce Plugin System to Core (at least the part that deals with the cache system conversion). Other than us needing to unclobber the $conf['cache_backends'] variable, please flag anything else you're concerned about there. Otherwise, it might get committed while you're moving without you having had a chance to review it, and that would be sad. Thanks.