Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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?
Comment | File | Size | Author |
---|---|---|---|
#18 | 1820526-18.patch | 2.05 KB | pdrake |
#14 | 1820526-14.patch | 2.06 KB | damiankloip |
#13 | 1820526-13.patch | 1.73 KB | damiankloip |
#11 | 1820526-11.patch | 2.66 KB | damiankloip |
#5 | 1820526-5.patch | 3.06 KB | damiankloip |
Comments
Comment #1
tim.plunkettThe second views_get_view should be redundant, right?
Otherwise, I agree with the change.
Comment #2
damiankloip CreditAttribution: damiankloip commentedThanks 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.
Comment #3
tim.plunkettOkay, 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
Comment #4
damiankloip CreditAttribution: damiankloip commentedYep, I like that idea. Patch to follow!
Comment #5
damiankloip CreditAttribution: damiankloip commentedComment #6
tim.plunkettThis is much more clear.
Comment #7
dawehnerThis looks great!
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.
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.
Comment #8
webchickMoving down to "needs review" based on Daniel's feedback.
Comment #9
damiankloip CreditAttribution: damiankloip commentedFollow 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?
Comment #10
dawehnerYes 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.
Comment #11
damiankloip CreditAttribution: damiankloip commentedHere 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.
Comment #12
damiankloip CreditAttribution: damiankloip commentedComment #13
damiankloip CreditAttribution: damiankloip commentedI 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.
Comment #14
damiankloip CreditAttribution: damiankloip commentedTim noticed I left in the UuidFactory property!
Comment #15
tim.plunkettRTBC if this passes, the code follows the docs and other entities in core.
Comment #16
xjm$config_test
does not look like an accurate variable name.Comment #17
aspilicious CreditAttribution: aspilicious commentedNo its not. Why not something simple as "view"?
Comment #18
pdrake CreditAttribution: pdrake commentedI updated the variable name to $view as that seems both simple and accurate to me.
Comment #19
pdrake CreditAttribution: pdrake commentedOops, should have set this to needs review.
Comment #20
tim.plunkettCopy/paste--
Thanks for fixing it @pdrake!
Comment #22
xjm#18: 1820526-18.patch queued for re-testing.
Comment #23
xjmAlright, this is good now: http://qa.drupal.org/pifr/test/377483
Thanks @pdrake!
Comment #24
catchOK 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!
Comment #25
dawehnerWe don't have to care about return value of $view->save(); as it is either new or updated.
Comment #26
dawehnerUps.