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.
Goal
- Leverage APC when it is available.
Use-cases
-
Symfony\Component\ClassLoader\ApcUniversalClassLoader
- Allow to toggle it from settings.php.
- See also: #1658720: Use ClassLoader instead of UniversalClassLoader
-
Drupal\Core\Cache\ApcBackend
- Allow to use it for assorted cache bins.
-
APC user variables? We surely must have use-cases in core that could leverage those?
Related issues/projects
Comment | File | Size | Author |
---|---|---|---|
#123 | 1807662-122.patch | 13.36 KB | Wim Leers |
Comments
Comment #1
msonnabaum CreditAttribution: msonnabaum commentedNot sure I care if we have cache backends for it, but I do like the idea of using apc when it's available. For example, this patch is quite helpful when your files directory is on a distributed file system:
https://github.com/pressflow/7/pull/16/files
Comment #2
cweagansI would argue that we should automatically use ApcUniversalClassloader if APC is available. Don't make me edit a configuration variable when APC is clearly the better option. I would also argue that we should automatically use ApcBackend when APC is available and that both of those options should be able to be overridden in settings.php.
Comment #3
Crell CreditAttribution: Crell commentedExcept that APC class loader would need a cache flush every time you touch a file or class name in dev. Ideally if we got a dev/prod toggle enabled we could auto-select the APC class loader if available in prod mode only. Related: #1537198: Add a Production/Development Toggle To Core
Comment #4
msonnabaum CreditAttribution: msonnabaum commented@cweagans - Using APC as a cache backend by default is insane. No way we're doing that.
Crell is right about the autoloader. It's something you have to be aware of, so it shouldnt be the default.
Comment #5
andypostWhy APC? xcache2 more powerful
How it could work on shared hosts with different config options?
Comment #6
podarokagree with #5
we should not integrate one op-code cacher feature - as framework has to be compatible and able to connect to any of them via common api layer
Comment #7
sunAdded Cache\ApcBackend from APC module.
Cleaned up apc stub functions.
Removed APC statistics support.
Fixed phpDoc, relocated methods.
Added visibility to methods.
Fixed missing method visibility in Cache classes.
Other Cache classes clean-ups.
Basic upgrade of getMultiple() and set() methods.
Added ApcBackendUnitTest.
Fixed bogus @file declaration.
Fixed bogus phpDoc in DatabaseBackend.
Upgraded ApcBackend for cache tags.
ApcBackendUnitTest passes, but UnitTestBase is broken; added lots of todos and debugging code. :(
We should probably back out the unrelated fixes into a separate issue.
Comment #9
sunMoved those fixes into #1808112: Missing method visibility, bogus phpDoc and coding style in Cache backend classes
Comment #10
sunRemove unrelated changes, and Merged in HEAD.
Comment #12
sunRemoved Simpletest debugging code. To be fixed over in #1436684: Remove static cache in drupal_valid_test_ua()
Comment #13
sunSorry, that was the wrong patch. :-/
Comment #15
brianV CreditAttribution: brianV commentedWe would need to be cautious about how this is done in order to not be detrimental in shared hosting situations where the default apc.shm_size (if enabled at all) may not be sufficient. Excessive cache empties and reloads don't help anyone.
I would argue that we don't want full caching in APC by default; perhaps limit it to Symfony\Component\ClassLoader\ApcUniversalClassLoader by default so as to not eat up too much of the segment size while still providing a good performance boost.
Then make the cache backend for cache bins optional, for the user to configure as they see fit (and have apc segment memory available.
On a more philosophical note - why should the cache backend be in core? Anyone savvy enough to configure APC (admittedly simple) and configure the bins in settings.php is savvy enough to download it from contrib. What do we gain by providing the backend in core? And why just APC and not Xcode2, memcache, eaccellerator etc. etc.?
Comment #16
sunAPC is maintained by the PHP core team. It is very actively maintained. It is available for all platforms and often comes pre-bundled/pre-installed even. Most of Drupal's competitors have built-in APC support since ages. Enough said.
Fixed unit tests are not able to detect PHP CLI, since run-tests.sh masquerades as Apache.
Fixed run-tests.sh bogusly detects unmet requirements as fatal error.
Comment #17
cweagansThis probably needs a comment in default.settings.php describing how to configure different cache bins to use APC with some examples (like the database settings). If the documentation isn't included in default.settings.php, then at least a URL for the documentation page on drupal.org should be added.
Comment #18
Crell CreditAttribution: Crell commentedJust to be on the safe side, this should probably be static::, not self::, so that we get late static binding. There's very few reasons to still use self:: in PHP 5.3.
Can't we externalize this logic? Two global calls here make me cringe.
Again, can't the prefix be externalized? It's basically the same thing as passing in a DB connection to the database-backed cache driver.
I hereby decree this method shall be pronounced "Binky". :-)
I'm curious why you're doing the fancy functional approach for deleteMultiple() but not for flush()? It's not a blocker I guess, I'm just curious why you wouldn't do both the same way.
Why is this code outside of the class? That's a no-no.
Comment #19
brianV CreditAttribution: brianV commentedMost of Drupal's competitors are inferior to Drupal. That's hardly a justification for extra core bloat.
Don't get me wrong - I like APC a lot. I use it on all my projects. I don't, however, see any good justification for adding more core bloat... Given that the cache backend is intended to be pluggable, I ask again - why are we putting this backend in core, and not contrib? Alternatively, what justification allows this one in core, but not other stable caching backends?
Comment #20
alexrayu CreditAttribution: alexrayu commentedDon't bloat the core up people!
Comment #21
cweagansFirst, you need to define bloat. Disk space is cheap and I could care less about the few extra kilobytes that this would add. This code won't get loaded if it's not used, so if you don't want it, don't turn it on.
Second, having this available in core (especially if/when the config option is added to default.settings.php) ensures that people actually know about it. One of the biggest complaints that I hear about Drupal is that it's slow. I ask them "Well, have you turned on all the caching options built into core? Memcache? Boost? APC?" They usually answer, "Well, no...I haven't even seen that stuff."
Third, APC is the most common caching mechanism available, so this makes more sense for core than, say, the memcache backend.
Personally, I think it would be great if Drupal could cache things somewhere other than the database without having to download another contrib module. A module being enabled seems like more resource usage on every page request than simply setting a flag in settings.php
Comment #22
brianV CreditAttribution: brianV commentedProbably a good idea. I would consider this bloat because it's code that is in core, and bound by core policies, that doesn't need to be. It places a burden on the core devs and those who help maintain core...
There is definitely a problem here, but I see the solution in better documentation and training, personally.
Anyways, I will admit, we are off into the weeds from my initial main concern - I suggest we don't enable the APC cache bins by default, apart from perhaps class loading. Lots of shared hosting has 32M (default) or smaller apc.shm_size, and it's not hard to overrun that especially if the user runs several sites on the same account. The problem I see is that when APC is full, it flushes itself *completely*.
So if you end up filling the cache every few requests, the users could end up with a cold cache as often as not. It might actually be detrimental to performance in these situations.
Comment #23
sun@Crell:
Yeah, most of that prefix/bin/key code originates from the APC module and I didn't really touch it yet, since the logic going on there is quite advanced and debugging showed that some parts of it are indeed necessary.
The static stuff is definitely not required though - I'll convert that into a simple class property that's set up only once in __construct().
Regarding "externalizing" (I guess you mean injecting?) the prefix, I guess that's what #1764474: Make Cache interface and backends use the DIC is about?
The binKey() method should go away entirely IMHO - it just prefixes the $cid with a string - I don't see why we'd need a function call for that.
The key() method should be renamed to addPrefix(), I think.
The difference between deleteMultiple($cids) and flush() is that the former gets a known list of $cids passed, so those keys can be simply prefixed. flush(), however, needs to find all cache items in the store that belong to the prefix.
Lastly, the stub functions are a very interesting detail in this. (Apparently, a stub for ApcIterator is missing.) As the comment clarifies, APC might be turned off for PHP CLI but may be set as cache class via
$conf['cache_classes']
in settings.php, in which case stuff like Drush and other CLI scripts would attempt to use it and break with fatal errors. Do you have any ideas for how we could resolve that in a better way?Comment #24
cweagansRead the patch. This is not enabled by default. You have to put stuff in settings.php to turn it on. Also? Shared hosting typically does not have APC enabled.
Comment #25
sunCleaned up APC prefix determination, variables, and methods.
Comment #26
sunRenamed all 'Apc' to 'APC', since it's an acronym and PHP itself uses 'APC', so 'Apc' would be inconsistent.
Comment #27
sunCreated testbot issue: #1809248: Enable apc.enable_cli php.ini setting on testbots
Comment #28
andypostAPC is a trademark :)
Comment #30
Crell CreditAttribution: Crell commentedstatic:: please. :-)
Hm. OK, I see what is going on here, now. I'm not a fan of the approach, but I don't have a better idea yet. At minimum, this sort of hackery should go in common.inc or something rather than in a class file.
And yes, by "externalize" I mean inject, vis, move that logic external to this class. If we can't do that yet due to another issue, let's drop a @todo on it with an issue link so we know to fix it later.
Comment #31
sunMe too. This code also originates from APC module. (It's really helpful to move something into core that was battle-tested already.)
The downside of moving this code elsewhere, however, is that we'd non-trivial code for checking whether the stubs are/will be needed. I'm perfectly aware that this completely violates PSR-*, but keeping the stubs in there seems to be the most simple and most elegant way to solve the problem, because the code is only loaded when actually needed and doesn't introduce complexity nor code debt elsewhere.
...but apparently, an APCIterator stub is still missing, and trying to add that in the same way yields:
So here's a different proposal that hopefully works better:
1) Check availability in the constructor, set a Boolean property, and implant a condition into the public cache API methods. If FALSE, do nothing.
2) Optional, later: If FALSE, turn the APCBackend silently into a MemoryBackend (i.e., built-in), so as to prevent data from being rebuilt multiple times on CLI.
Replaced self:: with static::.
Replaced stub functions with APCBackend::$available property.
Comment #33
Crell CreditAttribution: Crell commentedAh, I like that a lot better. Agreed that a memory backend fallback would be even better. Could we do that by just making this class subclass off of the memory backend, and then
if (!static::$available) { return parent::whatever(); }
?Comment #34
sunAdded fallback to MemoryBackend if APC is not available.
Added test for APCBackend fallback to MemoryBackend. (YAY!)
Comment #36
pounardComment #37
Damien Tournoud CreditAttribution: Damien Tournoud commentedYou don't even need multiple frontend. The same problem occurs between the command line and the web server.
Comment #38
sun@pounard:
1) You obviously did not read any issue comments, nor earlier patches, so there's no point in responding to your available/MemoryBackend fallback criticism.
2) Regardless of being injected or not, an externally callable getPrefix() method will be required to figure out the prefix. Even more so, when the prefix needs to be injected. Moving it into a completely separate function outside of the class would only make sense if the identical logic can be applied for other k/v cache backends, which I'm entirely not sure about.
3) PHP uses APCIterator, this is the APCBackend. There's no Symfony involved here. Consistency is thus to be achieved with PHP. In any case, if there's disagreement on that detail » separate issue, thanks.
4) Challenges involving multiple web heads are heavily advanced and can be happily follow-ups to this patch, if needed. Much like the challenge of a possible PHP extension on CLI that's required for the backend, these are actually the secondary major reasons for why I want APC support built-in in core. Alternative implementations for other backends (but also APC) can exist in contrib and learn from the best practices we're figuring out in core.
Comment #39
pounardNot all that's true. But I did read some of them. Regarding "...but apparently, an APCIterator stub is still missing, and trying to add that in the same way yields" the contrib backend actually don't suffer from it, and in the continuity of the legacy method, we can stub quite easily:
class APCIterator extends EmptyIterator {}
and it will work seemlessly.Memcache and Redis are two examples that jumped out of my head in less than 3 seconds after reading that [EDIT: actually Redis was already in my mind since it's a topic in which I'm trying to get uniformisation accross contrib backends because core don't provide anything for it].
We are using Symfony too, both two are valid proposals, I'm just proposing the other one.
Calm down and respect multiple years of work and many issues solved by many users out there on the existing backend, and don't try to impose what are the "best practices", especially when you cut/pasted lots of code from them.
Providing an non working implementation as soon as you try to scale, without any warning about incomplete status of the backend is foolish. I don't want non Drupalistas to see that after running their site into prod, after they actually broke it because of it, just because it's "better to do in a follow up" which has 50% chances of never being finished.
In the end, my opinion about this issue is that it is the right one that triggers all those problems and that needs to fix it. The same way that each new use case of the K/V store leads to a K/V store patch in the specific issue, the new problems we discover here must be taken care of right when we found it. Especially when it's a series of long term problems that a lot of people actually are experiencing!
For the record, here are some issues where those problems are trying to be solved:
Comment #40
sunRegarding CLI vs. webserver (or any other SAPI interface):
A common approach to that seems to be to invoke scripts that require to access the same memory through the desired SAPI; e.g.:
There are probably other possible solutions. In any case, I don't really see why that should block us from adding APC support? And why that cannot be discussed in a separate follow-up issue? The problem surely applies to pretty much all alternate caches around, so discussing it on the sole ground of APC would be incomplete anyway.
Comment #41
Damien Tournoud CreditAttribution: Damien Tournoud commentedIt really only applies to APC (and similar shared memory cache schemes), which is cannot be accessed from outside the web server SAPI. Memcache, Redis, etc. are both cluster-wide and accessible via the CLI.
Comment #42
pounardWe cross-posted, I edited my post upper.
We can use the state API to store the latest cache clear timestamp and store its equivalent at flush() call in a key in the APC side. When the object is being built at __construct() time we can compare them and run the flush() if the state() version is higher than the APC local version. This is the most simple solution, and this is probably just something like 10 to 20 lines to implement, so let's do it once for all.
The state() key can actually be set either on a per bin basis, either globally, and can be used by other backends. We can even add a
LocalCacheBackendInterface
which only adds the getter and setter for the local value, which then leads us to be able to either:* put the backend behing a generic proxy implementation that does the check
* use the factory/manager to do it for us and leave the backend flush itself at __construct() time.
There is also numerous other smart leads proposed in the given links upper.
Comment #43
damiankloip CreditAttribution: damiankloip commentedOkay, Let's try to get this issue moving again! I think I agree that the APCBackend should not care about implementing the fallback logic itself. It should be a dumb cache implementation - this should be handled outside of the class.
I have rerolled the patch to remove all this logic from APCBackend, and also update it to the current CacheBackendInterface, as a few things have changed since the last patch. So atm I have just removed the prefix logic, as I think we need somewhere else for it to live.
Comment #45
damiankloip CreditAttribution: damiankloip commentedForgot about $allow_invalid
Comment #47
damiankloip CreditAttribution: damiankloip commentedSorry, also needs a deleteTags() method.
Comment #49
damiankloip CreditAttribution: damiankloip commentedComment #51
catchAPC is less likely to be available now that opcache is in PHP 5.5.
There's http://pecl.php.net/package/APCu which provides the user cache without the opcode caching but not sure how well that's going to be supported.
Comment #52
pounardIndeed. If D8 remains PHP 5.3 compatible, a lot of hostings will still ship APC (think of not-so-bleeding edge Linux distros for example) so I guess having this in core for 8 is still a good idea, while it will probably be removed for later versions. The amount of code needed for this remains minimal and encapsulated, and will be quite easy to maintain for 8.
Comment #53
PanchoI agree with pounard on that it's worth it and the good maintainability, plus I don't even expect D9 to rightaway require PHP 5.5, so it would stay in core until D10.
Comment #54
damiankloip CreditAttribution: damiankloip commentedI agree with the above, I think it's a nice little win, and it wont be a big overhead to maintain. I will work on this and get it passing.
Comment #55
cweagansRelated: #2029885: Bump minimum PHP requirement to 5.4.0
Comment #56
damiankloip CreditAttribution: damiankloip commentedThis will get a bit closer.
Comment #57
andypostso result of apc_store() does not matter?
Comment #58.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #59
sunGiven that PHP 5.5 has a built-in opcache now, and any D8 site that seriously cares for performance is going to run on PHP 5.5, I'd suggest to limit the scope of this issue to just a built-in
Drupal\Core\Cache\ApcuBackend
https://gist.github.com/msonnabaum/9836899 is a quick & dirty hack based on
MemoryBackend
, but might helpComment #60
Anonymous (not verified) CreditAttribution: Anonymous commentedi'll roll the work over in the inconsistent backend issue in to this one today.
Comment #61
catchComment #62
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #63
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #64
damiankloip CreditAttribution: damiankloip commentedRe #59, you mean limit the scope (keeping it similar to patches so far) and not include any logic to decide if/when this bin gets used or not?
Comment #65
catch@damiankloip that's in #2248767: Use fast, local cache back-end (APCu, if available) for low-write caches (bootstrap, discovery, and config) now.
Comment #66
Anonymous (not verified) CreditAttribution: Anonymous commentedok, here goes. this is pretty much just the code at #56, with the following changes:
* APCBackend renamed to ApcuBackend
* ApcuBackend->prefix renamed to ApcuBackend->sitePrefix
* ApcuBackend->deleteAll() now uses an APCIterator directly
Comment #67
sunLooks good to me. First wanted to post a review on many minor things, but then figured it's faster if I simply fix them myself.
Various clean-ups + Fixed bogus $prefix variable in deleteAll().
Comment #69
sun67: apc.cache_.67.patch queued for re-testing.
Comment #71
catchOpened #2250211: Enable apc.enable_cli for core APC cache backend tests.
Comment #72
Anonymous (not verified) CreditAttribution: Anonymous commentedassuming we get the bots to the point this can pass, i like changes in #67 - looks pretty ready to me.
Comment #73
Wim LeersIn this reroll:
ApcuBackendFactory
(with correspondingcache.backend.apcu
service), that usesconf_path()
as the default site prefix.::setMultiple()
.CacheBackendInterface::CACHE_PERMANENT
no longer equals zero, which the current code assumes; fixed.::removeBin()
deleted *everything* in APCu, rather than only its own entries; fixed.(To test, just change
$service_name = 'cache.backend.database';
to$service_name = 'cache.backend.apcu';
inCacheFactory
— on a fresh D8 install with no content, the number of DB queries should drop from 150 to 60 with warm caches.)I think the intent here was to do something with those CIDs for which no cache item was to be found? Not sure what the plan was, but right now, this line is rather useless :)
Comment #74
Wim LeersOops, #73 contains one file of another issue.
Comment #75
sun$cids
is passed by reference.Comment #78
Wim Leers#75: aha, overlooked that :) Alright, I have nothing else then.
This is now just blocked on #1809248: Enable apc.enable_cli php.ini setting on testbots, then it should come back green.
Comment #79
damiankloip CreditAttribution: damiankloip commentedChanges look good. However, just using conf_path() as a prefix is not unique enough as that would be E.g. 'sites/default'. You could have multiple sites on a server with the same conf path. I spoke to catch and alexpott in IRC about this and we agreed that using DRUPAL_ROOT too is a fair approach - then md5()ing this string to give us our site prefix.
Also changed the prefixing slightly as before it would be something like 'sites/defaulttags::', that's a little confusing. Now it would be something like '158a9d58b1d7e7fc53d11eeec068ef25::tags::'.
EDIT: Looks like the $cids = ... comment has been resolved too :)
Comment #80
damiankloip CreditAttribution: damiankloip commentedComment #81
Wim Leers#79: awesome! I figured
conf_path()
would be insufficient, but we'd then at least move forward. Thanks for figuring out the best way :)So now we're *really* down to waiting for #1809248: Enable apc.enable_cli php.ini setting on testbots to be fixed.
Comment #83
PanchoNice. However we might want to support both APCu and the new, alternative user cache YAC which performs better on reads and is also being packaged now.
Comment #84
catchThat'd need a different implementation, quick look at the github page says it has a key length restriction, which APC doesn't. So it'd need to be its own backend (either core or contrib - contrib for now since the project page says don't use it in production).
Comment #85
damiankloip CreditAttribution: damiankloip commentedAgreed, that seems very very much like something for contrib. Let's see where that project ends up further down the line.
Comment #86
Wim LeersThat and,
— even their warning has a typo. Definitely not yet coreworthy.
Comment #87
Anonymous (not verified) CreditAttribution: Anonymous commentedwe need to fix that :-)
each test run should use something that won't collide with other runs as the APC prefx.
Comment #88
MixologicIf I understand this correctly, we need to upload multiple copies of #79 in one issue so that we can shotgun the testbots so that we can try and get #1883 to grab it?
Comment #89
MixologicOkay. I rerolled and posted the patch from #78, and shotgunned the bot with a mass of them. (https://drupal.org/node/2255215)
The test still failed, but it definitely ran: https://qa.drupal.org/pifr/test/780063
Was this working locally? Is this still an environment issue? or were we expecting this to fail?
Comment #90
Wim Leers#89: it works fine locally, including the tests passing.
Comment #91
MixologicI'm inclined to believe that there is still something not the same environment wise between the testbot and whatever folks are doing locally.
It looks like 3 of the patches all hit the bot that was configured to run with apc, and all three had the same 11 errors. Perhaps we need rfay to let us know which version of APC is configured and what its php.ini settings are.
Comment #92
BerdirI can reproduce those test fails on 5.5 APCu, both when running on CLI and in the UI.
Comment #93
MixologicAdding a related critical beta target that will impact this.
Comment #94
damiankloip CreditAttribution: damiankloip commentedYeah, this issue is probably closer than that one I would say :)
Comment #95
Anonymous (not verified) CreditAttribution: Anonymous commentedmkay, so tags handling was just kinda borkenated forever with the earlier versions of this patch.
attached patch should fix that.
Comment #96
Anonymous (not verified) CreditAttribution: Anonymous commentedwith patches this time.
Comment #98
Wim Leers96: 1807662-95.patch queued for re-testing.
Comment #100
Mixologiclets try one more time to test #96 this time.
Comment #102
Anonymous (not verified) CreditAttribution: Anonymous commentedyay for this running on the bot now :-)
fixed some dodgy invalidate code.
Comment #103
sunI wanted to RTBC, but two tasks are still remaining:
'test'
should be$this->databasePrefix
.Alternatively, if we want to pass what's actually taken, then
$this->siteDirectory
(=conf_path()
during a test).Both variables are holding unique test class/site specific strings.
Lastly, this test should have a
tearDown()
method that force-purges the entire user storage (at minimum for the aforementioned test site prefix).Comment #104
Anonymous (not verified) CreditAttribution: Anonymous commentednew patch addresses #103.
Comment #105
sunAwesome, thanks! :-)
Comment #107
Wim LeersThe move of the cache tag metadata deletion from
removeBin()
todeleteAll()
got me thinking. I started talking to beejeebus in IRC, and we came to the joint conclusion that the cache tag implementation of this cache back-end is subtly broken.The cache tags are being stored not a per-bin basis, but on a per-site basis:
It must be site-wide, because
::checksumTags()
usesstatic::$tagCache
(just like theDatabaseBackend
) , which means it's being statically cached for all APCu cache bins within this site. But if that's the case, then doing this in::removebin
is wrong:It was wrong before #104 (and I introduced it, so I'll take blame), and it's wrong since #104, because both
::deleteAll()
and::removeBin()
are bin-specific operations, yet they delete the site-wide (and thus covering N APCu cache bins) cache tag metadata for APCu cache bins.I think there are three possible solutions:
DatabaseBackend
does precisely this also. This is probably the easiest and most correct course of action.::removeBin()
operation is very rare, we could consider it acceptable that in that case, all cache entries in all cache bins of that type are invalidated because the cache tags checksum will no longer match.::checksumTags()
. (Sincestatic
implies site-wide, rather than bin-specific.)All of this, because I introduced this in #73 — sorry!
This explains the test failure of #104.
Comment #108
olli CreditAttribution: olli commented#107: would #2250033: Garbage collection for cache tag invalidations solve it if we do 1. here?
protected?
Could use a shorter $ttl ($expire is REQUEST_TIME + $ttl).
Could this use the getAll() like apc_delete($this->getAll())?
I think these should set $data['value']->expire and use apc_store instead of $this->set().
These should have to reset static $tagCache...
This never sets the $tag key but the prefixed $tag key.
Comment #109
Anonymous (not verified) CreditAttribution: Anonymous commentedi went with 1. from #107.
the only thing i agreed with from #108 was 1.
Comment #110
Wim Leers#109 actually implements #107.2, not #107.1.
Comment #111
damiankloip CreditAttribution: damiankloip commentedI think we should go with option 1 and leave the tag data intact - keeping it in line with how we do stuff in the db backend at the moment.
Comment #112
Anonymous (not verified) CreditAttribution: Anonymous commentedreroll coming today...
Comment #113
damiankloip CreditAttribution: damiankloip commentedliar!
Comment #114
Anonymous (not verified) CreditAttribution: Anonymous commentedam not! am not!
the diff is:
Comment #115
pwolanin CreditAttribution: pwolanin commentedMinor, but md5() and sha1() should not be added to Drupal core in 7.x+
Comment #116
moshe weitzman CreditAttribution: moshe weitzman commentedThat policy got reversed with the introduction of Symfony. There isn't much point in preserving it for older releases IMO. Or at least suggest a workaround if you are going to downgrade a serious performance issue.
Comment #117
damiankloip CreditAttribution: damiankloip commentedYes. You just did the same in another issue for no apparent reason. Let's not base our code on some static code analyser. I guess that's your reason? For a cache key there are not really security implications. These aren't passwords. Plus using sha1 is perfectly sufficient as far as collisions go and to create a site prefix md5 is not going to hurt us at all.
Comment #118
Wim LeersFrom #107:
As of #114, this is the case. Back to RTBC per #105, since the main reason this didn't get committed already is because I marked it NW in #107.
Fixing #115 at the same time, after consulting pwolanin in IRC about what should be used instead. Fixed 3 nitpicky nitpicks at the same time. I agree with #116/#117 that this is pointless, but I don't want to delay this patch any longer, so doing this to avoid further bikeshedding.
Comment #120
sunI agree with @moshe + @damiankloip. The wildcard "Don't use
md5()
/sha1()
anywhere" idea (which never was a formal policy btw) has caused a lot of damage and long-term incompatibilities. It's obsolete.Comment #121
pwolanin CreditAttribution: pwolanin commented@sun. I strongly disagree. I also opened at least one issue to fix symfony. It was certainly a policy or we would not have removed every use from Drupal 7 before release.
The fact that some vendored libraries are not up to this standard does not mean that Drupal core itself should regress.
Comment #122
damiankloip CreditAttribution: damiankloip commentedWhat are the implications of not using this 'standard' again? I see no point in #115. This is not a policy so we should not be treating it like one. Doing this in d7 also got a lot of kickback - for good reason.
Comment #123
Wim LeersFailed due to a
core/lib/Drupal/Core/Test/TestKernel.php
hunk I've got NFI of how it got there.Otherwise identical.
Comment #124
damiankloip CreditAttribution: damiankloip commentedSorry.
Comment #125
Crell CreditAttribution: Crell commentedThe md5/sha1 issue is because some government procurement policies forbid them as insecure, and any use of them needs extra special justification to show why they're not a security vulnerability. Avoiding them entirely reduces the paperwork involved in getting governments (or at least the US government, which is a huge user of Drupal) to adopt Drupal.
Comment #126
Wim LeersI think #125 clearly articulates the sole valid reason for not using
md5()
in this scenario: government bureaucracy. Not everyone may agree that it's worth taking into account, but it's a fact that it's irrelevant for this patch how we create the hash.Again, let's not have this discussion in this issue. This issue has been delayed enough as is. I've opened a new issue where this can be discussed and formalized in an actual policy: #2268875: [Policy, no patch] Using md5()/sha1()/crc32b in Drupal code.
#123 uses the "safer" hash functionality provided by the
Crypt
utility, the discussion can move to #2268875, hence this issue is absolved of any further discussion on this matter. Back to RTBC.Comment #127
damiankloip CreditAttribution: damiankloip commentedok, fair enough *grumble grumble* :) The patch is looking just fine.
Comment #128
damiankloip CreditAttribution: damiankloip commentedHang on, why do we need to base64_encode() this hash for the site prefix too?
Comment #129
Wim Leers#128: we don't need that. But this is a ready-to-use hash function, and there's no harm in base64-encoding it. The size of the resulting hash is similar to when we used
md5()
. Let's not get stuck in bikeshedding over that too.Comment #130
Dries CreditAttribution: Dries commentedGreat! I'd much prefer this over using PhpBackend so it is a great option to have. Committed!
Comment #132
Wim LeersHurray! :) Now let's get #2248767: Use fast, local cache back-end (APCu, if available) for low-write caches (bootstrap, discovery, and config) done, so that Drupal will actually use this cache backend automatically if possible!
Comment #133
olli CreditAttribution: olli commentedThis gives me a PHP Warning: #2269453: apc_fetch() expects a string or array of strings.
Comment #134
damiankloip CreditAttribution: damiankloip commentedI'm sorry, I just don't understand this, what about all hash functions already built in to php? I do no see a problem with just using
hash('sha256', $this->sitePrefix)
. Also, that is not bikeshedding (because you wanted it rtbc?!), it's just a valid point.Comment #135
olli CreditAttribution: olli commentedFiled #2272685: Incorrect handling of invalid cache items in apcu backend for #108.2-6.