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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pounard’s picture

@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.

pounard’s picture

Status: Postponed » Needs review
FileSize
18.87 KB

I know this is postponed, but here is a 20mn work that should definitely work.

Status: Needs review » Needs work

The last submitted patch, 1323120-cache-to-namespace.patch, failed testing.

pounard’s picture

Oups forgotten added files...

pounard’s picture

Status: Needs work » Needs review
FileSize
35.06 KB

Retrying with the right patch...

Status: Needs review » Needs work

The last submitted patch, 1323120-cache-to-namespace-fixed.patch, failed testing.

pounard’s picture

Status: Needs work » Postponed

The 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.

pounard’s picture

Status: Postponed » Needs review
FileSize
35.22 KB

Update patch, I'm not convinced it will pass tests, but at least it works flawlessly on my test site.

Status: Needs review » Needs work

The last submitted patch, 1323120-cache-to-namespace-3.patch, failed testing.

pounard’s picture

Status: Needs work » Needs review
FileSize
34.32 KB

Same but removed useless code in install.core.inc, hoping this was the source of problems.

Status: Needs review » Needs work

The last submitted patch, 1323120-cache-to-namespace-4.patch, failed testing.

Crell’s picture

+++ b/core/includes/Drupal/Cache/InstallBackend.php
@@ -0,0 +1,66 @@
+/**
+ * A stub cache implementation to be used during the installation process.
+ *
+ * The stub implementation is needed when database access is not yet available.
+ * Because Drupal's caching system never requires that cached data be present,
+ * these stub functions can short-circuit the process and sidestep the need for
+ * any persistent storage. Obviously, using this cache implementation during
+ * normal operations would have a negative impact on performance.
+ */
+class InstallBackend extends DatabaseBackend {

Can 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.

pounard’s picture

The 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.

aspilicious’s picture

+++ b/core/includes/cache.incundefined
@@ -1,18 +1,21 @@
+ * ¶

trailing whitespace

23 days to next Drupal core point release.

neclimdul’s picture

Patch applies cleanly for me so not sure what the test bot's problem is. However... seeing some weirdness. Will report shortly after some investigation.

neclimdul’s picture

Getting 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.

pounard’s picture

Oh 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.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
36.8 KB

A reroll with all the doc changes

Status: Needs review » Needs work

The last submitted patch, 1323120-cache-18.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review

#18: 1323120-cache-18.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1323120-cache-18.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
36.78 KB

Fixed doc error

Status: Needs review » Needs work

The last submitted patch, 1323120-cache-21.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review

#22: 1323120-cache-21.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1323120-cache-21.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
18.52 KB

attached patch just fixes a type in cache(), which was still referring to DrupalDatabaseCache instead of Drupal\Cache\DatabaseBackend.

Status: Needs review » Needs work

The last submitted patch, cache-psr0-1323120-26.patch, failed testing.

Anonymous’s picture

i can't reproduce the fail the testbot is reporting locally :-(

Anonymous’s picture

Status: Needs work » Needs review
FileSize
37.01 KB

ahem, this time with the new class files.

Status: Needs review » Needs work

The last submitted patch, cache-psr0-1323120-29.patch, failed testing.

pounard’s picture

Merf, 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.

pounard’s picture

Drupal\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.

Anonymous’s picture

call_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?

Anonymous’s picture

Status: Needs work » Needs review
FileSize
37.02 KB

soooo. when in a namespace, catch (Exception $e) doesn't work, we need catch (\Exception $e).

aspilicious’s picture

WTF...

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?

catch’s picture

Should be a use statement at the top of the file per coding standards.

pounard’s picture

Yes, 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.

aspilicious’s picture

FileSize
37.18 KB

Well 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...

aspilicious’s picture

FileSize
37.15 KB

Whoops newline problem

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

awesome, i think use Exception is much better.

sooo - i think this is RTBC, it's mostly just moving / renaming.

pounard’s picture

Ok 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 !

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/Drupal/Cache/BackendInterface.phpundefined
@@ -0,0 +1,172 @@
+ * @see DrupalDatabaseCache
+ */
+interface BackendInterface {
+

Could 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.

+++ b/core/includes/Drupal/Cache/DatabaseBackend.phpundefined
@@ -0,0 +1,273 @@
+  /**
+   * Implements DrupalCacheInterface::isEmpty().

Don't we need to update these comments? I see similar comments updated elsewhere.

+++ b/core/includes/install.core.incundefined
@@ -277,8 +277,7 @@
-  $conf['cache_default_class'] = 'DrupalFakeCache';
+  $conf['cache_default_class'] = 'Drupal\\Cache\\InstallBackend';
 
   // Prepare for themed output. We need to run this at the beginning of the
   // page request to avoid a different theme accidentally getting set. (We also

Double quotes should work here instead of the escaping?

pounard’s picture

Could we maybe make this CacheBackend instead of BackendInterface

It could:

I'm not sure what the standards are in terms of naming stuff like this.

I am not sure myself, I'd say there is none about naming. We should ask Crell maybe.

Double quotes should work here instead of the escaping?

\ 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.

Don't we need to update these comments? I see similar comments updated elsewhere.

Yes we definitely need, doing it!.

pounard’s picture

I'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?

catch’s picture

If we do that we'll need an issue to update the coding standards, it does seem redundant with class per file.

pounard’s picture

Status: Needs work » Needs review
FileSize
37.25 KB

New patch attached, from #38:

  • Renamed BackendInterface as CacheBackendInterface
  • Fixed all Implements ... comments
  • Removed the @file docblocks (they do not serve any purpose here)
  • Did some minor typo fixes (really a few)
pounard’s picture

@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.

pounard’s picture

@#42 http://drupal.org/node/608152 states that Interfaces should always have the suffix "Interface".

aspilicious’s picture

Status: Needs review » Needs work

Every file needs an @file statement even small css files. You can not remove it.

pounard’s picture

@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.

catch’s picture

As I said in #45, we should open a new issue for the coding standards change.

pounard’s picture

@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.

catch’s picture

OK 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.

pounard’s picture

Status: Needs work » Needs review
FileSize
37.98 KB

Added 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.

pounard’s picture

FileSize
37.99 KB

After aspilicious review, I fixed multiple problems he spotted:

  • Missing new line after class declaration and first class member
  • Remaining last DrupalDatabaseCache reference

Status: Needs review » Needs work

The last submitted patch, cache-psr0-1323120-55.patch, failed testing.

pounard’s picture

Status: Needs work » Needs review

What?!

pounard’s picture

Oh my... error! Fixing it right now.

pounard’s picture

FileSize
37.99 KB

Always forgot to increment the line numbers when manually editing a patch file, fixed^^

pounard’s picture

Yay! RTBC?

jbrown’s picture

It isn't necessary to escape backslashes. It looks really terrible.

'Drupal\\Cache\\DatabaseBackend'

can be

'Drupal\Cache\DatabaseBackend'

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.

pounard’s picture

It isn't necessary to escape backslashes.

\ 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... :

To specify a literal single quote, escape it with a backslash (\). To specify a literal backslash, double it (\\). All other instances of backslash will be treated as a literal backslash: this means that the other escape sequences you might be used to, such as \r or \n, will be output literally as specified rather than having any special meaning.

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 looks really terrible.

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.

Crell’s picture

Hm, 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. :-)

pounard’s picture

Thanks :)

catch’s picture

Status: Needs review » Reviewed & tested by the community

All 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.

jbrown’s picture

Before 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.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

It 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:

+   * @todo: This method is deprecated, as its functionality is covered by more
+   *   targeted methods in the interface.
+   */
+  function clear($cid = NULL, $wildcard = FALSE);

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:

+ * This is more than a simple stub implementation: it also removes real caches
+ * from database when clear/del method are called. This allows the installer to
+ * drop stalled cache entries created during site installation AJAX requests
+ * done over the incomplete site pending installation.
+ *
+ * This class future is to be dropped as soon as the incomplete caches being
+ * built during installation are fixed. Do not use it elsewhere.
+ */
+class InstallBackend extends DatabaseBackend {

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

pounard’s picture

@#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.

pounard’s picture

Status: Needs work » Postponed

This 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.

pounard’s picture

Status: Postponed » Needs work

The 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.

pounard’s picture

I diff'ed and fixed the patch on my side, running tests locally then I will send it later in the afternoon.

pounard’s picture

FileSize
36.34 KB

Here is a new patch attached.

  • Removed the clear() method cruft from all backends
  • Fixed the InstallBackend to fit with the new delete*() methods
  • Moved the InstallBackend::clear() method code documentation onto the class docblock
  • Removed the poor gramatical comments, a bit redundant with original one that replaces it
  • Ran the Cache* tests @home, they pass, we'll see for full core

Acid critiscism and sharp eyes are needed!

pounard’s picture

Status: Needs work » Needs review
aspilicious’s picture

+++ b/core/includes/Drupal/Cache/InstallBackend.phpundefined
@@ -0,0 +1,109 @@
+ * same time (for example, Ajax requests triggered by the installer). If we
+ * didn't try to clear it whenever this function is called, the data in the
+ * cache would become stale; for example, the installer sometimes calls

This isn't a function anymore so it needs a rewrite. Else it is confusing.

-4 days to next Drupal core point release.

pounard’s picture

True, let's see if the patch passes tests and I will fix it.

pounard’s picture

FileSize
36.38 KB

Fixed the docblock as this:

+ * If there is a database cache, this backend will attempt to clear it whenever
+ * possible. The reason for doing this is that the database cache can accumulate
+ * data during installation due to any full bootstraps that may occur at the
+ * same time (for example, Ajax requests triggered by the installer). If we
+ * didn't try to clear it whenever one of the delete function are called, the
+ * data in the cache would become stale; for example, the installer sometimes
+ * calls variable_set(), which updates the {variable} table and then clears the
+ * cache to make sure that the next page request picks up the new value.
+ * Not actually clearing the cache here therefore leads old variables to be
+ * loaded on the first page requests after installation, which can cause
+ * subtle bugs, some of which would not be fixed unless the site
+ * administrator cleared the cache manually.

Changed first sentence and the one aspilicious showed.

Review time!

Crell’s picture

#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. :-)

aspilicious’s picture

FileSize
36.38 KB

I can do this

pounard’s picture

Good, thanks!

sun’s picture

Issue tags: +PSR-0
catch’s picture

Status: Needs review » Reviewed & tested by the community

OK I read through again, the regression of the other patch is completely removed now, and everything else looks good.

Third time lucky?

sun’s picture

Raised 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.

pounard’s picture

Aside 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.

aspilicious’s picture

FileSize
36.38 KB

Just converted the double backslash to a single backslash as discussed in the meta issue.

catch’s picture

Title: Convert cache system to PSR-0 » Change notification for: Convert cache system to PSR-0
Priority: Major » Critical
Status: Reviewed & tested by the community » Active

Thanks! 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.

Crell’s picture

Congratulations pounard for getting the first Drupal\ namespaced code in Drupal. :-)

webchick’s picture

Status: Active » Needs review
FileSize
744 bytes

First; Yay!

Second:

-  $conf['cache_default_class'] = 'DrupalFakeCache';
+  $conf['cache_default_class'] = 'Drupal\\Cache\\InstallBackend';

Missed one. :)

pounard’s picture

@Crell I was not alone, everyone that participated in here ows the merit :)

pounard’s picture

@catch I might need the "revisit later" tag for the PHP doxygen following the comment http://drupal.org/node/1320648#comment-5464260 ?

David_Rothstein’s picture

FileSize
3.21 KB

While 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.

kotnik’s picture

FileSize
2.97 KB

Due to changes in #1400748: Proposal for unified namespace organization here's fixed patch witch changes from #87, #90 and one more.

Status: Needs review » Needs work

The last submitted patch, drupal-1323120-91.patch, failed testing.

xjm’s picture

Title: Change notification for: Convert cache system to PSR-0 » Convert cache system to PSR-0
Priority: Critical » Major

The current task here isn't writing the change notification; please re-tag etc. when the code is settled. :)

catch’s picture

Status: Needs work » Needs review

#91: drupal-1323120-91.patch queued for re-testing.

neclimdul’s picture

@kotnik is there a reason that the install fix disappeared from your patch?

kotnik’s picture

@neclimdul, could you, please, write what install fix do you mean?

catch’s picture

@kotnik: see webchick's patch from #87.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.83 KB

@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 :)

catch’s picture

Title: Convert cache system to PSR-0 » Change notice for: Convert cache system to PSR-0
Priority: Major » Critical
Status: Reviewed & tested by the community » Active

OK committed/pushed this one to 8.x, I think we're back at change notice now.

cosmicdreams’s picture

If all this needs is a change notice should this issue be critical?

aspilicious’s picture

Yes, we make them critical so people actually write them :)

webchick’s picture

Title: Change notice for: Convert cache system to PSR-0 » Convert cache system to PSR-0
Priority: Critical » Major
Status: Active » Fixed

Courtesy of core office hours! http://drupal.org/node/1479568

xjm’s picture

Lovely 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).

Status: Fixed » Closed (fixed)

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

effulgentsia’s picture

Status: Closed (fixed) » Needs review
FileSize
677 bytes

Can 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'].

pounard’s picture

Agree, 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.

Berdir’s picture

Hm.

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.

pounard’s picture

Berdir 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.

effulgentsia’s picture

FileSize
6.37 KB

Thanks. 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.

catch’s picture

I'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.

effulgentsia’s picture

That'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.

catch’s picture

Hmm. 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!

effulgentsia’s picture

Status: Needs review » Fixed

I 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.

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