Posted by larowlan on March 14, 2013 at 9:19am
9 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | views.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | VDC |
Issue Summary
Coming in from #1907960-114: Helper issue for "Comment field" where we have tests that add fields and need a method to force flush the views data cache.
Patch to follow
Comments
#1
Same as from the original issue, but with tests.
#2
Not sure how useful this is, I don't think there's a partial rebuild, so if it's missing something, it just rebuilds the whole thing I think.
#3
@Berdir this very useful in case when some entity needs update it's relations
+++ b/core/modules/views/tests/views_test_data/views_test_data.views.incundefined@@ -25,7 +25,13 @@ function views_test_data_views_analyze(ViewExecutable $view) {
+ $state = state();
+ if (!($count = $state->get('views_test_data_views_data_count'))) {
+ $count = 0;
+ }
+ $count++;
+ $state->set('views_test_data_views_data_count', $count);
+ return $state->get('views_test_data_views_data');
$count logic needs comment about it's purpose
#4
+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.phpundefined@@ -256,4 +256,20 @@ public function destruct() {
+ if ($key) {
+ unset($this->storage[$key]);
+ $this->cacheBackend->delete($this->baseCid . ':' . $key);
Sorry, if I was unclear, I meant the partial delete is not useful.
Adding a delete() makes sense, and existing calls that directly clear the cache tables should probably switch to calling delete() instead.
If you look at the get() implementation, if it can't find a table, it just rebuilds the whole views data information. So there's no point in trying to support this, a simple delete() is enough.
#5
Removes partial functionality as indicated by @berdir.
Adds comment as requested by @andypost.
#6
+1 to RTBC, Suppose someone from VDC need approve the patch
#7
This looks good to me, removing the selective from the issue title :)
There might be some direct cache clear calls in views that should switch to this?
#8
This should be tagged with VDC, then us guys will see it :)
+++ b/core/modules/views/lib/Drupal/views/Tests/ViewsDataTest.phpundefined@@ -53,6 +53,22 @@ public function testViewsFetchData() {
+ $count = $state->get('views_test_data_views_data_count');
Do we need to get this first and do $count + 1? I know this is more flexible, but are we ever going to change the starting count to anything but 0? so this could just assertEqual against 1.
+++ b/core/modules/views/lib/Drupal/views/Tests/ViewsDataTest.phpundefined@@ -53,6 +53,22 @@ public function testViewsFetchData() {
+ $this->viewsDataCache->get($table_name);
Is it worth us testing getting all tables too?
+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.phpundefined@@ -256,4 +256,11 @@ public function destruct() {
+ * Allows a module to flush the storage and cache.
Not sure about saying module here, how about just "Clears/resets the class storage and cache"?
+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.phpundefined@@ -256,4 +256,11 @@ public function destruct() {
+ public function delete() {
I think this should be clear() instead? or berdir also suggested rebuild()?
+++ b/core/modules/views/tests/views_test_data/views_test_data.views.incundefined@@ -25,7 +25,16 @@ function views_test_data_views_analyze(ViewExecutable $view) {
+ return $state->get('views_test_data_views_data');
We don't need to return anything here?
#9
Sorry, a couple of my comments are just plain wrong:
I guess other things could invoke this etc.., you never know. So I agree this is a safer option.
Duh, we were already doing that! I misread... I actually added that originally which is the bad part :)
Anyway, rerolled with some of those changes to hopefully speed it along and rectify that poor code review.
#10
I suggested delete() instead of clear() in order to match more the CacheBackendInterface(). We could it also name deleteAll().
#11
I spoke to dawehner about this on IRC, he also agreed that clear() as a method name is good to go.
#12
Just let's use Drupal::service('state') instead of state().
#13
Committed/pushed.
Also opened #1947720: Use Drupal::state() to replace state().
#14
Automatically closed -- issue fixed for 2 weeks with no activity.