Problem/Motivation

While reading the memory backend code I got confused that we serialize/unserialize the data there. I cannot imagine that there is a proper usecase for it.

References. If we don't, then we might end up with changing cached data by reference (objects), that's not something that you'd expect as a user of the API.

Proposed resolution

Modify \Drupal\Core\Cache\MemoryBackend (core/lib/Drupal/Core/Cache/MemoryBackend.php)'s class docblock. At the end (just before @ingroup cache), add some lines explaining why ::prepareItem() and ::set use unserialize() and serialize().

Remaining tasks

Write a patch!

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#12 interdiff.txt732 bytesHeyLodyM
#12 2538956-12-modify-docblock.patch773 bytesHeyLodyM
#10 interdiff.txt937 bytesHeyLodyM
#10 2538956-10-modify-docblock.patch772 bytesHeyLodyM
#8 2538956-8-modify-docblock.patch741 bytesHeyLodyM

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

To be clear, I *think* that is the reason and trying to make sure it behaves the same as an external cache backend.

We could also look into not doing that and define it as a feature, if start using it more and more as a drupal_static() with cache tag support replacement.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I also remember that being the reason.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Document that MemoryBackend::get/set uses unserialize/serialize to fix references. » Document that MemoryBackend::prepareItem()/::set() uses unserialize()/serialize() to break references
Issue summary: View changes
Issue tags: +DevDaysSeville

@HeyLodyM is taking on this issue at Drupal Dev Days Seville!

Clarified issue title and specified remaining tasks.

HeyLodyM’s picture

HeyLodyM commented
Assigned: Unassigned » HeyLodyM
HeyLodyM’s picture

HeyLodyM commented
Status: Active » Needs review
StatusFileSize
new 2538956-8-modify-docblock.patch741 bytes

I added the missing documentation. What do you think?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.php
@@ -10,6 +10,11 @@
+ * The functions ::prepareItem()/::set() use unserialize()/serialize().
+ * It behaves as an external cache backend to avoid changing the cached data
+ * by reference. During prepareItem, the object is not modified because we make
+ * a clone of it.

The first sentence ("The functions…") looks great.

The second sentence ("It…") should start on the same line as the first one: we must fill it up to consume <=80 characters. Other than that, it looks great.

The third ("During…") is a bit confusing. I think "During" can be replaced by "In". I think "prepareItem" should be changed to "::prepareItem()". And I think "is not modified because" should be expanded to "is not modified by the call to unserialize() because".

When you address this feedback, please provide an interdiff (see https://www.drupal.org/documentation/git/interdiff), to make it easy for reviewers (like me!) to see what has changed between the previous patch and the new patch.

HeyLodyM’s picture

HeyLodyM commented
Status: Needs work » Needs review
StatusFileSize
new 2538956-10-modify-docblock.patch772 bytes
new interdiff.txt937 bytes

@Wim Leers, thank you for your advice, I created a new patch and the interdiff file.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

Just one stupid silly small thing:

+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.php
@@ -10,6 +10,11 @@
+ * The functions ::prepareItem()/::set() use unserialize()/serialize().It

".It" needs to be ". It"

Sorry!

HeyLodyM’s picture

HeyLodyM commented
Status: Needs work » Needs review
StatusFileSize
new 2538956-12-modify-docblock.patch773 bytes
new interdiff.txt732 bytes

It's corrected!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Component: documentation » cache system
Priority: Normal » Minor
Status: Needs review » Reviewed & tested by the community
Issue tags: +Documentation

Woot, thank you! :)

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed 
+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.php
@@ -10,6 +10,11 @@
+ * reference. In ::prepareItem(), the object is not modified by the call of to

Sorry one more nit, 'of to'.

+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.php
@@ -10,6 +10,11 @@
+ * reference. In ::prepareItem(), the object is not modified by the call of to

One nit which I fixed on commit 'of to' -> 'to'.

And yes this is the reason and we should definitely document it explicitly.

Committed/pushed to 8.4.x and cherry-picked to 8.3.x, thanks!

  • catch committed 5eaf705 on 8.4.x 
    Issue #2538956 by HeyLodyM, Wim Leers: Document that MemoryBackend::...

  • catch committed 82e231e on 8.3.x 
    Issue #2538956 by HeyLodyM, Wim Leers: Document that MemoryBackend::...

Status: Fixed » Closed (fixed)

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