A good point raised in http://drupal.org/node/1805996#comment-6626626, we had this code in the load method originally for a reason (which I forget). However, I'm not sure this is helpful here anymore. A reason for this initially was that we don't want to ship default views with a uuid, but if they are subsequently saved, we want to assign one.

Move this to a save() method on the controller instead?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewStorageTest.phpundefined
@@ -143,7 +149,9 @@ protected function loadTests() {
+    $view = views_get_view('frontpage');
+    $view->save();
     $view = views_get_view('frontpage');

The second views_get_view should be redundant, right?

Otherwise, I agree with the change.

damiankloip’s picture

Thanks for the review.

I think we want that, then it properly tests the process of saving a default view that has no uuid, saving it, then loading it again to make sure it then has this new uuid.

tim.plunkett’s picture

Okay, then let's just add a comment saying that we're loading it again on purpose.
Or maybe even assert that it has no UUID on first load, that it does after save, that it does on second load, and that those two match.

Like

$view = views_get_view('frontpage');
$this->assertFalse(isset($view->storage->uuid));
$view->save();
$saved_uuid = $view->storage->uuid;
$view = views_get_view('frontpage');
$loaded_uuid = $view->storage->uuid;
$this->assertIdentical($saved_uuid, $loaded_uuid);
damiankloip’s picture

Yep, I like that idea. Patch to follow!

damiankloip’s picture

FileSize
3.06 KB
tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +VDC

This is much more clear.

dawehner’s picture

This looks great!

+++ b/core/modules/views/lib/Drupal/views/ViewStorageController.phpundefined
@@ -58,6 +43,19 @@ protected function attachLoad(&$queried_entities, $revision_id = FALSE) {
+    if (empty($entity->{$this->uuidKey})) {
+      $uuid = new Uuid();
+      $entity->{$this->uuidKey} = $uuid->generate();
+    }

Just in general i'm wondering whether the default storage controller should work like that. You don't want UUIDs for default config by modules.

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewStorageTest.phpundefined
@@ -143,9 +149,14 @@ protected function loadTests() {
+    $view = views_get_view('frontpage');

We should probably make a small novice follow-up to decouple these tests from the existing default views, so we don't have to enable the node module in the future.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Moving down to "needs review" based on Daniel's feedback.

damiankloip’s picture

Follow up issue for the tests: #1821524: Decouple use of default views in ViewStorageTest

So you think we should move this issue/fix to the Config entity base class?

dawehner’s picture

Yes i kind of think that you always will have default config entities per module which doesn't have UUIDs in code,
but i agree that we could do that later.

damiankloip’s picture

Status: Needs review » Postponed
FileSize
2.66 KB

Here is an issue to add uuid to config entities on save #1822700: Add UUID to Configuration entities on save()

Here is a new patch that just removes this from Views. I will leave in the UUID tests, as these should still work fine.

I will postpone this for now on that issue, then test the patch if/when that is in.

damiankloip’s picture

Title: Move UUID generation code from load() method in ViewStorageController » Remove UUID generation code from load() method in ViewStorageController
damiankloip’s picture

Status: Postponed » Needs review
FileSize
1.73 KB

I closed #1822700: Add UUID to Configuration entities on save() and added hook_config_import_create to assign a UUID when default views are imported. Solves our problems.

damiankloip’s picture

FileSize
2.06 KB

Tim noticed I left in the UuidFactory property!

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

RTBC if this passes, the code follows the docs and other entities in core.

xjm’s picture

+++ b/core/modules/views/views.moduleundefined
@@ -79,6 +79,19 @@ function views_pre_render_view_element($element) {
+  $config_test = entity_create('view', $new_config->get());
+  $config_test->save();

$config_test does not look like an accurate variable name.

aspilicious’s picture

Status: Reviewed & tested by the community » Needs work

No its not. Why not something simple as "view"?

pdrake’s picture

FileSize
2.05 KB

I updated the variable name to $view as that seems both simple and accurate to me.

pdrake’s picture

Status: Needs work » Needs review

Oops, should have set this to needs review.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Copy/paste--

Thanks for fixing it @pdrake!

Status: Reviewed & tested by the community » Needs work
Issue tags: -VDC

The last submitted patch, 1820526-18.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
Issue tags: +VDC

#18: 1820526-18.patch queued for re-testing.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Alright, this is good now: http://qa.drupal.org/pifr/test/377483

Thanks @pdrake!

catch’s picture

Status: Reviewed & tested by the community » Fixed

OK I didn't like the way the new hook returns FALSE instead of just returning TRUE when it does something, but these are all going away with #1806178: Remove code duplication for hook_config_import_*() implementations of config entities so went ahead and committed this as is. Thanks!

dawehner’s picture

Status: Fixed » Reviewed & tested by the community
+++ b/core/modules/views/views.moduleundefined
@@ -79,6 +79,19 @@ function views_pre_render_view_element($element) {
+  $view->save();

We don't have to care about return value of $view->save(); as it is either new or updated.

dawehner’s picture

Status: Reviewed & tested by the community » Fixed

Ups.

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