Problem/Motivation
Now that entity schema can change under the hood views has to react as otherwise the views will be broken.
Support the following cases:
- rename base table
- rename data table
- add a data table
- add a revision table
- remove some fields
- Support the following transactions:
// base <-> base + translation + // base + translation <-> base + translation + revision + // base + revision <-> base + translation + revision + // base <-> base + revision + // base <-> base + translation + revision
Proposed resolution
Listen to the events fired by the entity storage. With this we can iterate over the existing views and update them
properly automatically.
Remaining tasks
User interface changes
API changes
Postponed until
Beta phase evaluation
Issue category | Bug because you don't want to have broken views if you update an entity type providing entity types |
---|---|
Issue priority | Critical because you don't want to end up with broking views |
Disruption | No API changes at all |
Comment | File | Size | Author |
---|---|---|---|
#76 | 2341323-76.patch | 57.99 KB | plach |
#76 | 2341323-76.interdiff.txt | 908 bytes | plach |
#74 | interdiff.txt | 1.92 KB | dawehner |
#74 | 2341323-74.patch | 58.01 KB | dawehner |
Comments
Comment #1
catchComment #2
fagoIdeally, views integration would be built upon the table mapping only and thus automatically adapt. But how do we handle stuff that goes away that has been used, just mark it as broken? E.g. revision functionality or a certain field?
Comment #3
dawehner@fago
Well, sure I would love to have that, but I don't think this feasible in case we want to get some release at some point.
The steps forward for now are to store entity-type, entity-field and column name in the configured view in order to be able to update the view properly.
Is there some event triggered when the schema is changed?
Comment #4
xjmComment #5
fagoThat doesn't really sound simpler to me, but you know better :)
Not right now, but when we do #2332935: Allow code to respond to entity/field schema changes views could use the same event system as well to get notified on entity changes?
Comment #6
xjmTagging as a priority for an AMS beta sprint.
Comment #7
xjmMarking postponed on #2332935: Allow code to respond to entity/field schema changes.
Comment #8
daffie CreditAttribution: daffie commentedWas postponed on #2332935: Allow code to respond to entity/field schema changes
Comment #9
dawehnerAdding one of the subissues ... #2349553: Store entity field information in the views data
Comment #10
plachComment #11
dawehner.
Comment #12
xjmI think this can be active again now? Or is it postponed on something else?
Comment #13
dawehner#2349553: Store entity field information in the views data adds some important meta-data, which allows us to figure out what kind of entity types we have after the update.
Comment #14
xjmComment #15
dawehnerWorked on some tests, to at least somehow make an outline what should happend at the end of the day.
#2388467: Remove \Drupal\entity_test\EntityTestViewsData and update the annotation. is one small tiny issue which resulted from that.
Comment #16
dawehnerSome other cases we have to support:
Comment #17
fagoI'm not sure about that. Imo, that's something not related to "storage-adjustments" but cleaning up config based on module removals. Afaik, we do generally clean-up all config depending on a module when we uninstall it, so it should work via that?
Comment #18
dawehnerThat is valid, ... we can drop that test in case we simply don't want to take care about that.
It is just, what I came up with as a possible test case.
This will be fun, to even just get all the possible test cases done.
Comment #19
dawehnerWorked a bit on more test coverage.
As @plach said, we could leverage a couple of things of the EntityDefinitionUpdateTest ... extracted that into a dedicated
trait and used it in the test ...which is still not completed, but at least we do test something already.
Comment #21
xjmComment #22
dawehnerUpdate the IS for some cases we should better support.
Comment #23
dawehnerReroll and some initial work.
Comment #24
plachJust curious to see what the bot says. I will have a look to this later...
Comment #26
dawehnerWorked a bit on it, now tests pass now.
Current problem
Currently I'm a bit helpless what we can do in case a data table is added, aka. an entity type got marked as translatable.
At this context you get a call to
onEntityTypeUpdate(EntityTypeInterface $entity_type, EntityTypeInterface $original)
. In order to figure out which fields needs to be updated, you somehow need to generate the newtable mapping, but sadly there seems to be now way to get a new one.
$storage::getTableMapping()
allows you to pass along a list of field storage definitions, but there seems to be no api to generate one given an
$entity_type
(EntityManagerInterface::getFieldStorageDefinitions()
is nearly it, but it just allows you to pass along the$entity_type_id
. Is there a different way to generate a new table mapping?Comment #28
jhodgdonReally, if you have for instance a Node content type that is not translatable, and you suddenly decide it needs to be translatable, that causes the entity schema to change? ?!? That seems utterly ridiculous to me.
Comment #29
jhodgdonOh, maybe you're talking about: you have a module that defines entity type Foo. Then next week you update the module and Foo is now a translatable entity, so the schema changes. That seems more reasonable...
Comment #30
dawehnerWell yeah, that is basically the usecase. We want to ensure that we don't end up with totally broken views,
especially because it is not obvious for site builders what happens and how they could fix it.
Comment #33
plach@26:
At the moment we are working around that impossibility to generate a table mapping starting from an entity type + field storage definitions by temporarily changing the wrapped entity type in the storage class, see http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Entity/Sql/S....
#2274017: Make SqlContentEntityStorage table mapping agnostic should provide a clean solution for this, but I don't think it's worth postponing this on that.
Comment #35
dawehnerThank you @plach, this helped both in term of motivation as well as on the technical side.
We now handle the basic cases, not sure whether we should ever deal with
<onEntityTypeDelete()
for example.General reviews, comments on the test coverage etc. would be highly welcomed.˚
Comment #37
plachOverall this seems to be going the right way, I did not immediately realize it's still a work in progress so forgive some remarks :)
Missing PHP docs
I guess we could disable all views dealing with the deleted entity type in this case...
Are we sure there's nothing to do here? What would happen if a column were added or removed?
Will Views automatically take care of possibly fixing joins in this case?
Comment #38
dawehnerTrue, we could also drop some relationships/fields automatically.
At least for adding we should not anything, this is for sure. Removing
a field is a different case. Sadly removing every handler is problematic as there might be implicit dependencies somehow.
Yes, it " fixes" it, because the joins are always just defined implicit in the
hook_views_data()
anyway.Comment #39
plachWouldn't it be better to disable the view? Just in case the entity type reappears later.
Sorry if it wasn't clear, I was referring to the case of a field storage definition update, in particular if the storage definition schema changes. Not sure we should cover this case, just wanted to bring it up.
Comment #40
plachComment #41
dawehnerWhat could happen in this case? Would we magically move a field from the base table to the data table?
Comment #42
dawehnerTest coverage is not a lie!
Comment #43
jibranFound some doc issues.
Do we have to clear ViewsData cache here or not?
No need to pass NULL.
We are also unsetting it. So can you please update the comment as well.
Issue id please.
I think it should be = &$view->
doc block all messed up.
doc block missing.
Comment #44
dawehnerAmazing, so should we next time just put everything into a single function ;)
I'd argue that this event will just be fired if a module is installed, which invalidates views cache already.
I did it on purpose to make things more explicit.
Good point, fixed it.
I'm still convinced that forcing issues causes more harm than good, because people won't think or rather never mention the todo.
Fixed
Fixed all comments as well
Comment #45
plachA more in-depth review :)
We are missing the same cases for the revision table here.
debug leftover
This code is virtually identical, can we call a common function and just pass the table key name and the old/new values?
Surplus blank line.
Can we move this method call out of the loop?
Shouldn't we leave a broken handler in place, so people can figure out something is broken and maybe restore the field definition? I'd avoid performing irreversible changes under the hood.
Missing PHP docs :)
Missing capital U :)
Surplus capital U :D
Wouldn't it be safer to check actual view names?
Why don't we have an entity manager service in the test?
We should check the
langcode
field too: it will be moved from the base table to the revision table.:)
Comment #46
dawehnerThank you a LOT for the review. Here is just some small continuous work, still a lot is too be done.
There is a hell lot of code which basically just iterates over all tables, we can certainly improve / simplify the code a lot, but I would love that we first have the complete test coverage.
For now I added a todo so we don't forget to discuss it.
Sure
MEH
It is not only more secure, it is also more easy to read.
We do have one already, we just don't use it at the moment.
Add a fixme for now.
Comment #48
dawehnerbetter title.
Comment #49
dawehnerWe (plach, dawehner) worked together on that and discussed a lot.
We figured out which steps we want to support, so here is the working version for all changes we intend to apply onto the existing views.
One thing we aren't clear yet is how we handle if your entity type has no revision support anymore.
You could simply drop the corresponding fields / views, or maybe just disable them and rely on broken handlers. The later
one has the advantage that you can still look at your views and try to fix things.
Comment #50
plachThis is looking great, just a few notes:
I'd move this before index change detection, since it should be more performant.
Missing PHP doc
Is this still the case?
I thought we agreed on using views names :)
What about adding a few assertions in between to ensure we have the expected state after applying the fist set of updates?
Comment #51
plachWe are also missing a check that the storage is an instance of
SqlContentEntityStorage
and thus implements the table layout(s) we are assuming here. As discussed, we should just bail out if not.Comment #52
dawehnerTotally forgot about that, sorry.
Expanded test coverage found more bugs.
Comment #53
dawehnerRefactored the code a bit, to reduce some lines of boilerplate code.
Comment #54
plachLooks great, just a few more remarks and then we are ready to go :)
Mmh, this is a bit inconsistent with the rest of the code here.
Missing space after comma :)
I'd still prefer to avoid making stuff disappear without warning ;)
Missing PHP doc.
Can we remove this now? I think we discussed this in IRC.
Comment #55
dawehnerLet's adapt it, as talked about in IRC.
Sure.
Comment #56
dawehnerLet's not longer drop fields.
Comment #57
dawehnerThis is what you get if you have test coverage.
Comment #59
plachCool, I'd say this is ready to go.
Comment #60
yched CreditAttribution: yched commentedViewsEntitySchemaSubscriber uses FieldStorageDefinitionEventSubscriberTrait, but doesn't seem to actually do anything in any of the the corresponding onField*() methods ?
Comment #61
plachOh, right. We removed the content of
onFieldStorageDefinitionDelete()
in one of the latest iterations.Comment #62
dawehnerGood catch.
Comment #63
plachRTBC +1
Comment #64
plachI'm terribly sorry, I didn't notice this is not implementing
EntityTypeListenerInterface
, although it's implementing the related methods.Comment #65
dawehnerNo problem.
Comment #66
plachHopefully this will be the good one ;)
Comment #67
jibranCan we add a comment here?
Comment #68
plach@jibran:
#2411791: Provide empty methods rather than abstract methods in EntityTypeEventSubscriberTrait / FieldStorageDefinitionEventSubscriberTrait should allow us to remove that empty method.
Comment #71
dawehnerUps :)
Comment #72
plachNow for reals
Comment #73
alexpottAre these done?
Missing docblock
docblock is needs @param and @return.
This @todo looks very lonely.
Comment #74
dawehnerYeah the
@todo
statements are adressed in the meantime.Comment #75
plachUbernitpick: I think there's a surplus blank line here :)
Comment #76
plachZapped!
Also slightly reworded a comment.
Comment #77
dawehnerHa, there is always room for one more nitpick :P
Comment #78
alexpottCommitted 23e9d7f and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.
Comment #79
dawehnerhigh five @all
Comment #81
plachYay!
Comment #82
dawehnerRetrospective sky tagging.