Problem/Motivation
The views data structure currently directly looks like the table structure.
This works fine for the SQL query plugin, but it is problematic for differnet problems
- You cannot write a generic handler, it is ALWAYS coupled to SQL. Per default
it should rather be possible to not couple to SQL but rather just couple in case
you need the flexibility - Updating entity metadata changes the table structure, so the configured views
would have to be updated on the fly - For people implementing EQ_views would need another totally separate views data
structure, so also the configured views don't work together at all.
Another problem we have is that in case the entity schema changes, the views data schema changes at the same time.
At the same time the views configuration keeps the same, we end up with broken views, see #2341323: Adapt the references field / table names in views, when corresponding entity schema changes
Proposed resolution
As a first step, store the used entity types/entity fields in the configuration so we can use it both for potential better
views querying, but more important for now, use it in case we change the entity schema.
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#41 | 2349553-41.patch | 164.07 KB | pfrenssen |
#29 | 2349553-26-the-good-stuff-do-not-test.patch | 45.53 KB | pfrenssen |
#26 | 2349553-26.patch | 158.28 KB | dawehner |
#26 | interdiff.txt | 1.28 KB | dawehner |
#24 | 2349553-24.patch | 158.23 KB | dawehner |
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedSo IMO the first baby step is making sure our tables know the entity they are form. Currently this is only statically added to the base table.
Comment #3
xjmComment #4
dawehnerSo this is the first bit, add the needed information (entity type + field name) onto the views data.
Comment #5
dawehnerExpanded the unit test coverage a bit. It would be great to get just this bit in, once we have done this, we could
both experiment with the automatic updating of the views on schema change and iterate on the base schema.
Comment #6
dawehnerSome more work.
Comment #7
catchIf #2341323: Adapt the references field / table names in views, when corresponding entity schema changes is postponed on this shouldn't it be critical?
Comment #8
dawehnerTrue.
Comment #9
jhodgdonI took a look at the latest patch.
I'm a bit confused about one thing. It looks like both the 'entity_type' and 'entity_field' array elements are expected to be on all views field entries in the data definition -- various parts of the patch are looking for them, at least, and it's been added to the schema, I think.
But then in the EntityViewsData::mapFieldDefinition part of the patch, it seems to be only adding 'entity_field' to field entries:
While the 'entity_type' part is added at the table level in the getViewsData() method:
I am not seeing that addition in the schema?
And then there is this very confusing (probably wrong) bit in ViewsExecutable:
This seems to be setting an 'entity_field' entry on a field to the table's entity type??!? That is probably a typo? The fact that all the tests pass with this in there is a bit worrisome.
Comment #10
dawehnerThank you for your feedback!
Never said that this patch is perfect, I just want to keep momentum in general.
@see previous answer :) In order words, don't trust me :)
Comment #11
jhodgdonOK then, setting status appropriately, it looks like we need a new patch?
Comment #12
dawehnerWorked a bit on it, fixed the critical parts of the review.
I decided to make the views data itself as small as possible but just blow up the actual views config.
Wrote some tests to ensure the information is there at least.
To be honest the current patch primarily helps #2341323: Adapt the references field / table names in views, when corresponding entity schema changes
Comment #14
dawehnerOne less test failure, hopefully.
Comment #16
dawehnerFixed the remaining failures.
Comment #17
jhodgdonThis looks good!
So, I'm still a bit confused about one thing. The schema change in views.data_types.schema.yml adds both entity_type and entity_field properties to the views_handler schema definition. This means it applies to everything has a handler (area, field, sort, filter, argument).
But looking at all of the code changes, it seems like it really only applies to field handlers.
Is that a problem?
Also I think this change would need to be documented somewhere. We have an open issue about improving the documentation of hook_views_data() somewhere... which has gone nowhere due I think to lack of reviews and rerolls... but this change needs to update the hook docs so that it explains this new requirement.
Comment #18
dawehnerThank you for your feedback!
Well, the patch is not done yet, we have to potential update all of the existing views still, so it will be applied to filter etc. as well.
Comment #19
dawehnerAlright, so I decided to write a script which updates all existing views (see update-views.php.txt)
Comment #21
xjmTagging based on this blocking #2341323: Adapt the references field / table names in views, when corresponding entity schema changes.
Comment #22
dawehnerJust merged.
Comment #24
dawehner... and produced a merge error ...
Comment #25
fagoI think in the end we should build up the views data "table" structure based upon virtual entity fields only, but that's too enthusiastic for now. Encoding the metadata in the table structure is really good first step in this direction and helps to solve current problems, so +1 on that!
Reviews the patch - interesting to see that it also moves node.title to node_field_data from node - but that's fine and is probably caused by view regeneration? Else, I found some small glitches:
Unnecessary new lines.
Missing docs.
Comment #26
dawehnerGood catch! As explained personally, that was an existing bug in HEAD, so I fixed that.
Comment #27
dawehnerUpdate the IS
Comment #28
pfrenssenReviewed the script from #19 that was used to generate the patch.
$table_entity_map
:This finds
entity_test__field_test
andtaxonomy_term_hierarchy
, but both of these are fine, as they are describing a relationship and not a field. The oneliner cannot distinguish between those.This is a field alias, running users:uid through the Numeric filter plugin. Assuming this is correct, it would get its data from users:uid which is correctly converted.
convert_single_config()
looks solid. It is only skipping the creation ofentity_type
andentity_field
if there is no table, or the table has been blacklisted in$table_entity_map
which I verified.All systems go!
Comment #29
pfrenssenThis is a version of the patch from #26 excluding all the automatically generated config changes. This is a bit easier to review.
Comment #30
jhodgdonNew issue title made no sense to me... how about this one?
Comment #31
jhodgdonLooking at the latest patch... I have a few comments and questions:
a) In the automated part where the views configs are being added to, I'm a bit confused at some places where there is just entity_type added and not entity_field. Can you explain that? For instance:
Also
It seems like the view handler stuff should either have both entity_type and entity_field, or neither, not just entity_type -- what good would that do?
b) I'm also curious whether the script in #19 that updated the views configs produces the same result as if the view was re-created and exported from the Config module UI in all cases. I guess it would be interesting as a test to try at least one or two and verify? This patch is huge...
c) Another question I have about this patch is whether the Wizard code changes are sufficient. For instance, in the Node wizard, the only changes in the patch are in the defaultDisplayOptions() method. But this wizard also sets up filters in defaultDisplayFiltersUser(), and also sets up the title field in display_options_row(). Neither of these have the entity_type and entity_field added. Shouldn't they?
I think there are probably other places in other Wizard classes with the same problems (if this is actually a problem; I am not sure).
d) EntityViewsDataTest::assertField() might be more convincing if it also verified the entity_type was correct. That may also be missing from some of the other tests.
Comment #32
dawehnerThank you for your detailed review!
I do agree that this is not obvious. The reason why we don't have 'entity_field' here, is that we have just custom fields link the link fields,
which aren't real existing fields, but still exists on the corresponding table in
hook_views_data()
.I could certainly agree that renaming the base table is more an esoteric usecase, and we don't have any kind of support for that anyway yet. Do you think those examples should be dropped again, even we might in an ideal future, work on it?
One counter example is comment_views_data.node ... which provides a relationship from the comment to the linked entity. That one is on the data table,
which could be also dropped/renamed at some point.
Well, ... its a general problem, that the test views can't be rexported like that, because they rely on schema etc. which is sometimes manually/dynamically defined. @alexpott though for example has a script which resaves the existing default views, and this got applied at least once
for the various views config schema issues.
You are absolute right that this was not completed, damn wizard code. #2383157: Let views wizards use ViewExecutable::addHandler is intended to solve it in a way that we are keeping sane in the future. Let's add some dedicated test coverage here.
Well, ... the entity table is stored on the outer levels, so we have test coverage for that already.
Comment #34
dawehnerHa, we had a wrong schema!
Comment #35
jhodgdonRE #31(a), I am not in favor of adding things "in case we might need them later". So I think adding ['entity_table'] without ['entity_field'] to the things that aren't real fields is not a good idea now, if we don't need it now. It certainly let me to be confused about them!
RE point (d), if the field handler info needs to have ['entity_table'] defined on it, then let's test for it as well as for the presence of ['entity_field']. If it doesn't need this... but I think it does? ... then let's not put it there at all?
Also the interdiff uploaded in #32 seems to be from the wrong issue. :( and the interdiff utility failed on me :( too bad.
Comment #36
dawehnerI'm sorry
Haha, well, the full point of this issue is to make #2341323: Adapt the references field / table names in views, when corresponding entity schema changes possible ...
... we do have test coverage for that already, as written above. It would also make the assertion way worse to read,
because you would need more parameters and what not.
Comment #37
jibranI reviewed the patch haven't found anything questionable so RTBC
Comment #39
fagoNo idea what this change is about. Else, looks good to go to me else.
Comment #40
pfrenssenTest bot disagrees.
Comment #41
pfrenssenThere was a merge conflict due to #2382931: Drupal\field\Plugin\views\field\Field::access returns an object instead of the expected boolean getting in. Rerolled.
Comment #42
larowlanThis will make #2385443: Test that base entity fields on views respect field level access cleaner
Comment #43
jibranBack to RTBC
Comment #44
dawehnerJust in case someone is interested ... given that we have a factory we should just leverage that here, when we initialize another view, even
if we are part of the view executable already.
Comment #45
alexpottThis issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 7cd91c9 and pushed to 8.0.x. Thanks!