Use reflection for ViewExecutable::destroy()

That's a wonderful patch if used correctly. In general we should try to avoid internal behavior but test the general behavior, but sure there are great use-cases for it.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -13,7 +13,8 @@
-use ReflectionObject;
+use ReflectionProperty;
+
 use Exception;

Our standard is now to not `use` top level classes, just do new \ReflectionProperty inline. Its out of scope to fix these others, so this whole hunk should just go away (its not our fault ReflectionObject is not used)

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -1270,4 +1271,37 @@ public static function generatePermutations($parameters) {
+   *   The name of the property to return.
+   */
+  protected function getProtectedProperty($instance, $property) {
+    $reflection = new ReflectionProperty($instance, $property);

Missing @return

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewExecutableTest.phpundefined
@@ -291,23 +292,13 @@ public function testDestroy() {
+    unset($defaults['storage']);

This could use a comment

+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.phpundefined
@@ -9,6 +9,7 @@
+use ReflectionClass;

Same here, just remove this and do \ReflectionClass inline.

+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.phpundefined
@@ -281,6 +282,16 @@ class ViewExecutable {
+  public $area;

Do we want this to be NULL? Otherwise, public $area = array();

+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.phpundefined
@@ -1721,15 +1732,12 @@ public function createDuplicate() {
-        unset($handlers);

Do we really not need this now?

+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.phpundefined
@@ -1737,24 +1745,12 @@ public function destroy() {
-    $keys = array('current_display', 'display_handler', 'displayHandlers', 'field', 'argument', 'filter', 'sort', 'relationship', 'header', 'footer', 'empty', 'query', 'result', 'inited', 'style_plugin', 'plugin_name', 'exposed_data', 'exposed_input', 'many_to_one_tables');
-    foreach ($keys as $key) {
-      unset($this->$key);
-    }
-
-    // These keys are checked by the next init, so instead of unsetting them,
-    // just set the default values.
-    $keys = array('items_per_page', 'offset', 'current_page');
-    foreach ($keys as $key) {
-      if (isset($this->$key)) {
-        $this->$key = NULL;
-      }
+    $reflection = new ReflectionClass($this);
+    $defaults = $reflection->getDefaultProperties();
+    unset($defaults['storage']);
+    foreach ($defaults as $property => $default) {
+      $this->{$property} = $default;
     }
-
-    $this->built = $this->executed = FALSE;
-    $this->build_info = array();
-    $this->attachment_before = '';
-    $this->attachment_after = '';

YES. Thank you. This is great.

This is a thing of beauty. Ship it!

+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.phpundefined
@@ -281,6 +281,16 @@ class ViewExecutable {
+   * Stores the area handlers which are initialized on this view.
...
+   * An array containing Drupal\views\Plugin\views\area\AreaPluginBase
+   * objects.
...
+   * @var array
...
+  public $area;

It doesn't make sense, as we either store the header/footer/empty text, but not nothing?

+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.phpundefined
@@ -1721,15 +1731,11 @@ public function createDuplicate() {
-        unset($handlers);

Thought we should still unset $this->{$type} totally.

+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.phpundefined
@@ -1737,24 +1743,12 @@ public function destroy() {
+    $reflection = new \ReflectionClass($this);
+    $defaults = $reflection->getDefaultProperties();
+    unset($defaults['storage']);
+    foreach ($defaults as $property => $default) {
+      $this->{$property} = $default;

I can see the point in using it here, but I'm wondering whether reflection is good on actual runtime, as it might slows it down? I'm not sure how much, but that should be at least considered.

Thanks for cleaning it up.

The only thing which is missing now, is a review by crell or similar level.

+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.phpundefined
@@ -1728,24 +1724,12 @@ public function destroy() {
+    $reflection = new \ReflectionClass($this);
+    $defaults = $reflection->getDefaultProperties();
+    unset($defaults['storage']);
+    foreach ($defaults as $property => $default) {
+      $this->{$property} = $default;

This really is a nice refactoring and really adds maintainability of the code.

Mind you, I love this but that's a given. We need Crell to bless this... trick.

This looks good to me but I think it shouldn't be overused. There are some cases where it can be useful, but generally, tests should only test the public API of a class as the implementation is it's own business.

But, as in this case with ViewsDataCache, it's quite important that the inner implementation is correct as bugs could lead to rebuilds of the views data cache on every page load. The interesting thing is that it actually was broken, see #1941164: hook_views_data() rebuilt on every page view ? and this test would not have caught that bug as it makes the same "mistake" as I did and calls destruct() more or less directly.

The test coverage that I added to the path alias whitelist, which was broken too, simply does a direct cache()->get() call in a web test, to be sure that it works in a real environment, see PathAliasTest:testPathCache(). If we test the inner workings of a class, we assume that we know what it does anyway.

In another example, in the tests that I've written for state caching, I'm injecting a memory cache backend which is essentially a mock and allows to me check that the internal implementation does work as expected as I can verify when it's called and how and also influence what it returns.

So, all I'm trying to say is that I'm wondering if we should add a sentence or two to the docblock and explain why/when you should use this and when not.

Is there a reason why you don't added this comment to the property one as well?

I cannot speak to the changes in Views itself; those seem reasonable, I think. However, the introspective testing going on here makes me do a spit take. Most of Berdir's trepidation in #26 I share, but would go a step farther. The docblock on it doesn't go far enough in stating that "if you need this, it means your code is still not clean enough." (Which I believe to be the case.) Using reflection for this sort of test is, I'd say, better than having public methods or properties solely for testing purposes, but not by much. :-)

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewsDataTest.php
@@ -47,12 +47,21 @@ public function testViewsFetchData() {
+    // Test that this table info now exists in the cache.
+    $this->viewsDataCache->destruct();
+    $cache = $this->invokeProtectedMethod($this->viewsDataCache, 'cacheGet', array("views_data:$table_name"));
+    $this->assertIdentical($cache->data, $data, 'Table data has been retrieved from the cache.');

This is why you inject dependent objects. You can mock them.

Is there no way we can avoid this? Because this is a hack, no question. If we have to go this route the documentation should be much clearer about "if you need to use this, you're already doing it wrong". I'd still like to see a better explanation of why we can't refactor the code to not need this quirk in the first place.

At least here it doesn't seem to be an issue to inject the object and test directly on the injected object.

Sure, functional tests that test integrated functionality are fine. But if you're asking "was the cache updated" by introspecting an object, then you're not doing a unit test OR a functional test. You're doing a hacked-up test.

Yes there is a mock implementation: Drupal\Core\Cache\MemoryBackend

Let's improve the tests in one of the other issues, and just get this small piece in.

My personal oppinion is that proper mocked tests is the way to really know whether the code works, though
we also need tests for configuration. One possibility would be to call the same test code, once for a manual created instance, and once for the one in the container.

interdiff, please?

I don't really understand what dawehner is suggesting in #36. With a proper mocking framework (which simpletest lacks, but PHPUnit has!), you can setup a mock object on-the fly, configure it with "I expect this method to get called once", then pass it in to the object being tested. That way you can verify that the cache was in fact called.

@crell Sorry this was just a rerole, no actual change at all.

I can't say that it's "not allowed". I don't have that kind of karma. :-) I am saying, though, that "if you can't test it without reflecting, you're probably doing it wrong in the first place". I don't know enough about this specific code to say exactly what "right" is, but generally "inject it, and then mock it" is the better approach.

That's a great cleanup so for, though for sure, we have to continue working to improve the ViewExecutable class in the future.

Did some testing as well: https://gist.github.com/dawehner/5343845

Before:

0.000350 stddev: 8.5e-05

After:

0.000397 0.0001020

Considering that you don't have that many calls to destroy() this additional cost seems okay

That's not as horrific slow as I was afraid of.

Committed 8b386d9 and pushed to 8.x. Thanks!