Problem/Motivation
Drupal 7 had the clear separation between properties (node.title) and field API fields, like the node body.
The same logic was applied in views.
In Drupal 8, the difference between base fields (node.title) and configurable fields (node.body) basically vanished on the entity API level, but views is not entirely up to date so it still treats base fields like node.title differently, which is problematic because of various reasons:
- Translation support, as Drupal 7 did not provide translation support for base fields (entity properties)
- Cacheability of the rendering
- Access of the base fields, as Drupal 7 did not provide field access on base fields (entity properties)
On other issues, we've updated Field UI renderers so that they would work with base fields, and updated Views base table handling to always use the data table as the basis for views:
- #2407801: Views generic field handler does not work with base fields
- #2429447: Use data table as views base table, if available.
So now we can fix this problem.
Proposed resolution
In this issue, we will update uses of the basic Views field handlers on entity base fields to instead use Field UI fields. This issue covers views data field definitions that use the following handlers (other field types are handled as part of the parent meta-issue #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality and its subissues):
- string
- boolean
- uri
- numeric
- standard
- markup
There have been several critical questions along the way:
- Question: How do we implement "raw" data access, for stuff like views data export (assuming we should actually support that)?
Answer The answer for now is: you should get the data from the entities always.
- Question: How do formatters interact with rewrite?
Answer Rewrite is applied after the formatter, so site builders might have to configure field formatters to output less before rewriting the output in views.
- Question: Do we want to bring over all functionality of the fieldapi field handler? (skip itemsw, reverse ...)?
Answer We do by leveraging the existing field API handler.
- Question: We have to ensure that groupby/aggregate functionality is still working for these base fields. Note: this is a tricky thing in general?
Answer We manage to do that by leveraging the existing field API handler. There is now test coverage to ensure that this also works.
- Question: And this big one: How do we ensure that we don't run into a crazy performance decline?
Answer #1867518: Leverage entityDisplay to provide fast rendering for fields might be a solution for that -- would fix the existing performance problems for the Field UI fields.
Remaining tasks
- Ensure that translation support works as expected
- Review the patch
- Commit
- Work on the general meta (parent issue) to convert more field types
- Solve the potential performance problems
User interface changes
Base fields and Field UI fields will behave the same in Views. [On this issue, only some base fields are covered, however.]
API changes
Base fields and Field UI fields will behave the same in Views. [On this issue, only some base fields are covered, however.]
Beta phase evaluation
Issue category | Bug because Views with base fields in them cannot currently be translated and are violating access control on the entities. |
---|---|
Issue priority | Critical because multilingual is part of Core and also this is a security problem. |
Prioritized changes | Prioritized because it's a critical bug fix. |
Disruption | Disruptive for core/contributed and custom modules/themes because it will require a change of existing views configuration as well as some API changes, when you override existing handlers. |
Comment | File | Size | Author |
---|---|---|---|
#216 | interdiff.txt | 2.44 KB | dawehner |
#216 | 2342045-216.patch | 121.06 KB | dawehner |
#214 | 2342045-214.patch | 125.1 KB | joshtaylor |
#205 | interdiff.txt | 1.26 KB | dawehner |
#205 | 2342045-205.patch | 119.65 KB | dawehner |
Comments
Comment #1
jbrown CreditAttribution: jbrown commentedComment #2
jbrown CreditAttribution: jbrown commentedComment #3
jbrown CreditAttribution: jbrown commentedI'm going to bump this up to critical as the huge advantages of having fields in the base table are taken away by this problem.
Comment #4
amateescu CreditAttribution: amateescu commentedPlease take a quick look at https://www.drupal.org/node/45111 to see when the critical priority should be used :)
Comment #5
Gábor Hojtsy#2217569: Fields row plugin: Translation is non-uniform for base fields, Field UI fields, links; no way to choose "this row's language" ran into the same problem. IMHO this should at least be major (if not for something else that it blocks that major :). #2217569-44: Fields row plugin: Translation is non-uniform for base fields, Field UI fields, links; no way to choose "this row's language" also has pointers from @jhodgdon as to where each rendering is if that helps.
Comment #6
Gábor HojtsyComment #7
jhodgdonThe problem in both this issue and the other one is that the base fields on entities are being assigned (in hook_views_data() or now the equivalent EntityViewsData classes) to use the "standard" and "string" and similar Views basic field handlers. These field handlers do not know that their data is an Entity Field, they just know it's some data that came from a database query.
In which case, for #2217569: Fields row plugin: Translation is non-uniform for base fields, Field UI fields, links; no way to choose "this row's language", the data cannot be translated (only entities can be translated, not "some data from a query"), and for this issue, the data cannot be assigned a Formatter (only Entity Fields can be formatted with Formatters, not "some data from a query").
So what I think we need to do to fix this issue and unblock #2217569: Fields row plugin: Translation is non-uniform for base fields, Field UI fields, links; no way to choose "this row's language" is to make a new field handler for use in base entity fields, which will understand that the field is an Entity Base Field, and thereby allow it to be translated and to use a Formatter.
We actually may need several field handlers... the Views field handlers currently being assigned to entity base fields are:
- date
- language
- boolean
- uri
- numeric
- standard
- markup
- And then there are quite a few custom field handlers from individual entity types, like 'node' (used for the node Title field), 'node_type', 'node_language', 'user', etc.
I'm not sure whether all of them really need formatting and translation? Potentially so... Although maybe if we had formatters available we wouldn't need some of the custom field handlers at all? I'm not sure, haven't looked at them to see what they're all really doing.
Comment #8
Berdir@dawehner is the one who needs to comment on this.
I made a similar suggestion on the entity views data patches, but he was against it. The current field plugins for configurable fields are very complicated and slow.
Comment #9
Gábor Hojtsy@jhodgdon: those base field handlers have equivalents mostly as configurable field handlers too, no? Except markup and standard maybe?
Comment #10
dawehner@berdir
Hey, I thought you would be away
Just to be clear, in the original entity views data patch I wanted to keep the issue as much in scope as possible. Changing
our internal thinking to treat all fields the same would have been to much for one issue itself.
In general though we maybe really have to switch to all formatters and or keep the current functionality as fallback.
There are a couple of intersections/questions you could ask ... just to be clear I think discussing this with all kind
of other people is required.
the raw data and the formatted data as before?
In other words, having some proper strategy first would be really helpful.
Comment #11
dawehnerJust to be clear, every step towards entity query (which is one is also kinda one) is a good idea!
Comment #12
jhodgdonRE #9, that is a good point that there are already configurable/formattable handlers for many of the fields being assigned on the base EntityViewsData class. I also think that for 'standard' we can probably use the configurable formatters for Text and Number fields, as appropriate to the data type of the base field. However, there isn't anything probably for the entity-specific field handlers such as node, node_type, etc.
Comment #13
yched CreditAttribution: yched commentedRight, Views is the last part of Core that still treats base fields and configurable fields differently. Good legacy reasons for that, but that is still mostly stale logic.
It's true that the rendering pipeline for "Field API fields using formatters" in views is crazy slow.
#2346763: Improve views field rendering performance by caching entity display objects should have made it a little faster, but the real solution IMO remains #1867518: Leverage entityDisplay to provide fast rendering for fields, which would make "rendering fields in N view result rows" much more in line with the typical "rendering N entities in some view mode". The plan is here, we just never got to actually implement it :-/
Comment #14
jhodgdonSo... IMO the problem of "You cannot use a formatter on a base field" is an annoyance (as a site builder, you could probably work around it with field rewrites or something). But the problem of "the translation doesn't work the same for base fields and Field API fields" is a major if not critical bug that we absolutely have to fix, independent of the performance cost of doing so (see the screen shots at the top of #2217569: Fields row plugin: Translation is non-uniform for base fields, Field UI fields, links; no way to choose "this row's language").
I think we have two choices on how to fix the translation problem:
a) Making base field rendering/translation work the same as field API rendering (which is what is being proposed here).
b) Making field API translation work the same as base field translation, meaning that instead of going through all the current logic of the "field display language" (which loads the entity, translates it, and then renders the field from there), we would always display fields in the language of the views row (which is what happens now for base fields because Views doesn't know they're entity-related and has to just use the data in the row).
I am a bit ambivalent about which route to go -- both would at least make field API fields and base entity fields consistent as far as display language in a single view.
But if, besides translation and formatting, we also want the translation options for Views displays that use fields to behave the same as Views displays that use entity view modes, we have to do (a), because entity view mode views do allow you to display rows in a different language from the row language. And then we will also want to do #2217569: Fields row plugin: Translation is non-uniform for base fields, Field UI fields, links; no way to choose "this row's language", which will unify the settings for display language on the Display for both entity view modes and field-based views.
Thoughts?
Comment #15
Gábor HojtsyEntity views may very well display content in fallback. If an entity does not have a translation it should display in some fallback language. This is frequently used in multilingual countries to provide a full experience even if your primary language of choice is not available. Which may easily happen in Belgium or Switzerland for some popular examples. It would be pretty bad if the same could not be applied to field based views, because then you cannot make a list of those entities with the same logic in a block for example. So in my understanding we'll need to resolve the field rendering with language settings (not just in row language).
Comment #16
jhodgdonRE #15: So it sounds like you are saying it is essential for field-based views to have the same "display in a language other than the row language" behavior as entity-based views. I pretty much agree with you; in #14 I was just saying that it would be possible to at least make Field API fields and base fields consistent with each other (which is really bad right now because they're in the same views display) by removing the translation capability of the field-based views entirely. I'm not advocating doing that.
So if we do need translation capability on entity fields, then we need to incur the performance cost of making base fields render the same as field API fields. Fixing the performance would also be good on one of those other issues, but it's more critical to have them working, right?
Comment #17
Gábor HojtsyYeah I don't know how slow that will make Views, but sounds like it may be required for the feasibility of Views with fields. Would be great to get some committer feedback if these are expected to require lots of time / work, because incurring the performance regression to fix that bug and then expecting to resolve that sounds like a cascade of problems that we need to be prepared for. I'll try to discuss this with @alexpott soon (not this week, not at BADcamp). If someone is at BADcamp and can bring this up that would be fabulous :)
Comment #18
jhodgdonWe can always try assigning it to @alexpott for feedback... might need an issue summary update first though, since it has gone beyond just formatters.
Comment #19
jhodgdonBump. We still need to figure out what to do on this issue, so that we can also solve #2217569: Fields row plugin: Translation is non-uniform for base fields, Field UI fields, links; no way to choose "this row's language", which is postponed on this issue. Both Gabor and I think that this is a critical issue, because it is blocking the ability for Views displays to make any kind of sense in multilingual sites... It really does probably need to block release. So I'm marking this Critical for now at least.
I've updated the issue summary. How can we move forward on this? Do we need to confer with @alexpott or @catch on the plan? Is someone willing/available to work on this?
Comment #20
jhodgdonComment #21
dawehnerComment #22
dawehnerWorked a bit on that.
The approach was basically:
To be honest I'm quite suprised.
Comment #23
dawehnerHere is the missing screenshot.
Comment #25
dawehnerFixed a couple of more failures.
On general pain point, we do have the moment, is that from moving away from the special title handler we don't get linking for free.
One really good generic approach would be to have formatters for linking to entities, sadly this would clutter up the ususal field interface.
A different approach would be to add yet another option to the field handler, to also have a little checkbox to link to the appropriate content.
Comment #26
dawehnerForgot the interdiff ...
Comment #28
jhodgdonI do not think that it is even close to OK to lose the ability to link the Title field to the node, so if this patch does that, it isn't OK. I think about half the time when I make views I want a title field linked; the other half I don't, but losing that ability means you'd have to do a Rewrite on the field and specify the link destination, which is a lot harder than checking the "link this field to the node" box.
Also, in the patch, this looks like a mistake, or at best something that isn't part of this issue?
Another minor thing with the patch, in FieldPluginBase:
Needs a corresponding addition to the doc block for the new @param?
Comment #29
larowlanRelated #2385443: Test that base entity fields on views respect field level access
Comment #30
dawehnerSure, there was always a good reason why this was a special fish. I'm still not sure what approach would be sane here.
Well, FieldStorageDefinitionInterface doesn't have this method, so it failed my unit test.
Sure sure ...
Comment #31
yched CreditAttribution: yched commentedRight, there's no built-in, cross-field-type support for displaying fields as links to their entity, and now feels a bit late to add it.
[edit: also, wouldn't necessarily play good with all formatters]
The standard formatter for image fields has specific code to allow this, because that's a frequent use case for images. Maybe a similar thing could be added to the formatter for short strings ?
Comment #32
dawehnerAdded the related issue: #2387019: String field formatters cannot link to their parent entity
Comment #33
dawehnerMerged in the other patch for now, to see how many failures are left.
Comment #35
dawehnerJust a reroll for now.
Comment #37
catchPosted on the wrong issue, fixing metadata.
Marked #2385443: Test that base entity fields on views respect field level access postponed on this.
Comment #38
tim.plunkettComment #39
larowlanpoking this along
Comment #40
larowlanthis change resulted in the {user_roles}.roles_target_id being renamed to {user_roles}.roles
Which meant SessionHandler::read() had an invalid SQL query.
Not sure if this is the right fix, might blow up spectacularly - but installs locally.
Comment #42
BerdirDid not read this patch yet, but that sounds like it is related to #2248977: Complete support for multi-value base fields in ContentEntitySchemaHandler and use it for the user.roles field, but I have no idea why this issue results in a rename of those columns?
Comment #43
dawehnerOpened up #2392209: DefaultTableMapping::getFieldColumName is broken for base tables ... its a blocker, but its not a blocker to continue work here.
Comment #44
Gábor Hojtsy#2387019: String field formatters cannot link to their parent entity landed! Yay :)
Comment #45
dawehnerLet's ask the bot again.
Comment #47
plachComment #48
dawehnerSome more work.
AAAAAAAAAAh
Comment #50
dawehnerFixed a couple of tests and introduced to use the fieldapi field handler for more than just the node.title.
Now the amount of failures will go up a bit again.
Sadly we have no real way at the moment to solve the following problem:
You want to use a fields based view and render a specific field in the language of the result row.
Comment #52
dawehnerOkay, let's install again ...
Comment #54
dawehnerSome more work ... but things are tricky. For example the timestamp formatter allows to configure much less than the corresponding views handler.
In general I think we need to split up this patch potentially a lot.
Comment #56
jhodgdonSplitting up the patch into different field types makes a lot of sense to me.
Regarding time stamps, currently the EntityViewsData implementations for the Created time and other time/date fields are adding about 10 entries per field, for the Month, Year, Month/Year, etc. We really need to unify that anyway and definitely seems like splitting that off would be good.
Comment #57
dawehnerKind of a reroll, added another child issue. It is a vulcano of issues if we want to fix it properly.
Comment #61
jhodgdonDefinitely a volcano. :(
Comment #62
dawehnerNote: Currently this patch doesn't install because of the following message:
... This is caused by the problem that we changed the used views field plugin, but we haven't changed the configuration yet.
#2395613: Make it possible to configure the output of a boolean field on the formatter level is the logical next step in this issue though.
Comment #63
dawehnerAnother issue: #2399211: Support all options from views fields in DateTime formatters
Comment #64
dawehnerMerged in the patch from #2395613: Make it possible to configure the output of a boolean field on the formatter level for now to see how we can continue here.
This is not a lot of work so far.
Comment #66
dawehnerIn order to fix the failures in a sane way, #2399931: Generic entity api field handler should live in views module not in field module needs to be done.
Comment #67
dawehnerWe need more tags
Comment #68
slashrsm CreditAttribution: slashrsm commented#2399931: Generic entity api field handler should live in views module not in field module landed.
Comment #69
dawehnerAlright, let's see how horrible the reroll was.
Comment #71
dawehnerThe vulcano starts to splash!
Bumped up criticals:
* #2344151: Comment field access doesn't work if $items isn't present
* #2397495: Disabling 'Display all values in the same row' shows all values in all rows
Just a nicety which would be good to solve: #2402827: Extract ViewUnitTestBase and ViewTestBase::assertIdenti* methods into a trait
On top of that this fixes hopefully a good bunch of the criticals.
Comment #73
dawehnerAgain some more work.
Comment #75
dawehnerJust a reroll and one fix.
Comment #77
webchickSince this is security-related, tagging as D8 upgrade path.
Comment #78
dawehnerLet's postpone on #2407801: Views generic field handler does not work with base fields
Comment #79
Gábor Hojtsy#2407801: Views generic field handler does not work with base fields landed now. Woot.
Comment #82
dawehnerWorking on a reroll for now.
Comment #83
dawehner.
Comment #85
dawehnerLet's get the failures down.
I think it might be the right strategy to split this issue up into one per field type, so one for string, boolean etc.
Comment #87
dawehnerA few test failures.
Comment #88
Gábor HojtsyBased on your suggestion, continuing supporting the language field with entity field formatters in #2384863: Translation language base field handler should use views field handler, provide unified options, which is practically breaking that base field type out of here :) Hit some walls with access checking on fields which nulls out the rendered views. Help there would be amazing :) Thanks!
Comment #90
Gábor HojtsyBased on experienced in #2384863: Translation language base field handler should use views field handler, provide unified options this patch will cause problems on multilingual sites unless #2394883: Language setup for entity and field based rendering in views is independent, confusing UI, lacking test coverage is resolved as well, which is why I heightened that to critical. Field.php loads an entity and does not consider the result value as-is, so it needs to load the right language of the entity. Right now it does not support the most common result-row-language option for example. Work on this issue can go along but there will be multilingual problems/regressions until that is resolved as well.
Comment #91
dawehnerSome more "funny" debugging.
Comment #93
Gábor HojtsyThis does not actually resolve language selection, that is the realms of #2384863: Translation language base field handler should use views field handler, provide unified options, so retitling for correctness.
Comment #94
jhodgdonActually the title was referring to "you can't choose what language to display base fields in", not "the language fields have a problem". ;)
Comment #95
Gábor Hojtsy@jhodgdon: uhm, the issue summary has this: "Field UI fields have a setting that allows them to be displayed in a language that may be different from the views row language. Base fields' values cannot be displayed in a different language.". What does this patch do to resolve that? What this patch does by making the base fields use formatters like field UI fields is that you cannot in fact use the row language anymore, so it makes the bug different, it does not resolve it. That is why we need #2384863: Translation language base field handler should use views field handler, provide unified options, and why I removed the language reference in the title.
Comment #96
jhodgdonI thought that this patch would make base fields use Field UI rendering, which loads the entity (in the right language) and displays them? If it doesn't do that, we need another issue.
As far as I can tell, #2384863: Translation language base field handler should use views field handler, provide unified options and the other children of this issue are about making the Field UI formatters for specific fields have all the options that the Views formatters had, so we don't lose functionality (that one is about the various Language fields), right?
Comment #97
jhodgdonOh and I agree that we also need #2394883: Language setup for entity and field based rendering in views is independent, confusing UI, lacking test coverage, which is maybe what you meant to link, in order to actually have this work right for languages.
How about this title?
Comment #98
jhodgdonfix typo.
Comment #99
plachQuickly skimming through this to get some familiarity with the code. I noticed a few code style issues:
:)
:))
Surplus blank line ;)
Comment #100
dawehnerRerolled, fixed the feedback from #99, thank you @plach!
Comment #102
Gábor Hojtsy#2395613: Make it possible to configure the output of a boolean field on the formatter level just landed, so this will not apply and can be slimmed down now.
Comment #103
pcambraSome things I've found:
This might not be needed as protected but just variables in the setUp
node_type->id()
Contains
Shouldn't be a both a test_group_rows and a test_ungroup_rows view? seems like the tests are expecting the two of them.
Comment #104
dawehnerJust a reroll for now.
Comment #106
dawehnerFor now just a reroll + fixing of the existing schema issues (hopefully)
Comment #108
Gábor HojtsyFixing the rest of the schema fails I think.
Comment #110
dawehnerThank you for that!
Here is a WIP from adding actual revision support back, there are small bugs here and there.
Comment #112
jhodgdonThis is so close!
A few cosmetic glitches I noticed in the patch, and two real questions at the end:
a)
Duplicated line here?
b)
I think this needs one more space of indentation in the added line?
c)
I think the custom_false value here should be 'Blocked', not 'False'? Just guessing but...
d)
The @param needs a documentation line.
e) GroupRowsTest
Technically all of these need docblocks :(
f) This is not cosmetic... I noticed that EntityViewsData is still using a base handler for the date/time fields. Is that OK? Won't they also need to be translated to the right language? I just don't think the base handler is really going to work here... Maybe, but maybe not. We may need to test it.
g) There are still some specific EntityViewsData classes using their own handlers instead of the Field UI handlers for fields. Maybe they can be fixed in follow-up issues, but we need to not lose track of them. I'm seeing custom handlers still being used in:
AggregatorFeedViewsData
AggregatorItemViewsData
BlockContentViewsData
CommentViewsData
FileViewsData
NodeViewsData (only the Title field has been fixed; there are others)
TermViewsData
UserViewsData
Comment #113
dawehnerMhh you kinda see something different than me probably :)
Yeah you are right here.
Well, as long we haven't done #2399211: Support all options from views fields in DateTime formatters switching over is not that easy :)
Yeah no question, we can't solve everything in one go, but I would rather suggest to switch over step by step, while ensuring that we have good enough test coverage.
Comment #114
jhodgdonRegarding the big questions, I've moved those to another issue -- #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality -- for proper tracking.
I just discussed this with dawehner in IRC... Conclusion:
Since we now have that other issue, which lists all the fields in Core entity views data classes that are using non-Field-UI formatting, I think we can feel free to reduce this patch to only convert some of the base fields, and do the rest later in follow-ups. Not everything can really be done here anyway.
So, the current patch includes:
- language field defaults on EntityViewsData - but this is also handled on #2384863: Translation language base field handler should use views field handler, provide unified options (which is more complete since it also covers other entity-specific language plugins) -- could be removed from this patch
- standard, markup, boolean, and numeric fields in EntityViewsData itself (which I think would be good to leave in this patch, and anyway the patch would be empty without them. :) )
- NodeViewsData - title field -- but there are a lot of other NodeViewsData plugins that probably need to be addressed, so maybe that part could be moved to another issue too.
And as noted above, time/date field defaults in EntityViewsData are not part of this patch. These could definitely be done in another issue and can't really be fixed until #2399211: Support all options from views fields in DateTime formatters is done.
Comment #115
jhodgdonOne question. Since this patch is concerned (probably) only with the boolean, standard, markup, and numeric fields, would it make sense to convert the other uses of these in Core Entity views data classes? There are not many:
numeric:
- Used in EntityViewsData both for regular numeric fields and Entity Reference fields. [already in patch]
- Used in FileViewsData for fields on usage table: id, count
- Used in CommentViewsData for field comment_count, last_comment_uid
standard:
- Used in EntityViewsData both for regular text fields and Entity Reference fields. [already in patch]
- Used in FileViewsData for fields on usage table: module, type
- Used in CommentViewsData for fields field_name (on two tables), entity_type
markup:
- Used in EntityViewsData [already in patch]
- Used in UserViewsData
boolean is only in the base EntityViewsData class.
Comment #116
dawehnerAnswer to #115
numeric
Note: We talk about the
file_usage
table which is not a table controlled by the entity API, so ... we can't do anything about it.Note:For
comment_entity_statistics
we can apply the same argument as above.standard
Same point...
Opened a dedicated follow up for the signature bit: #2425193: Convert the signature field in user_field_data to the ordinary views field
Comment #118
dawehnerThis fix could be simple
Comment #120
jhodgdonAh, good points about those non-base tables. Will update the meta issue.
Comment #121
jhodgdonInterdiffs above look good, by the way!
Comment #122
dawehnerPerfect, less workarounds are needed.
Comment #123
jhodgdonOMG OMG OMG all the tests passed!
So, I took a very careful look at the entire patch again. It mostly looks great!
A few comments:
a)
There's one line in NodeViewsData:
This probably either needs to be removed, done now, or replaced with a @todo (not @fixme) and a follow-up issue.
b) In NodeViewsData, it currently (without this patch) uses the 'node' plugin/handler for the 'nid' field. The patch doesn't change that; only changes the title field from 'node' to 'field' plugin.
However, farther down in the patch, in one of the views configs, we have:
This seems inconsistent. I think we want to change this line near the top of NodeViewsData:
to use 'field'?
This other change in a views config looks wrong:
This is an *argument*, not a field, and I think it needs to stay at node_nid (which is an argument handler, not a field handler).
Both of these changes are in core/modules/node/tests/modules/node_test_views/test_views/views.view.test_node_revision_nid.yml by the way.
c) In the base EntityViewsData class:
I'm not sure we need that "Let's ..." comment, or what it means really. Why would they be links without the following code? It seems like maybe this is referring to the diff between old and new code, but that comment will survive after the old line of code is gone...
d) Left-over debug code:
e) This change to the link to entity/node setting looks wrong:
f) Why is this documentation being removed?
(and others)
g) In FieldTest again:
Not sure what this @todo is?
h) Still there
Um, this doesn't cover clickSort? ?? If you're going to remove this line you also need to remove another blank line above it... Applies to several spots just after this in the patch as well.
i) Why are you removing one-line docs from methods?
j) Line added to end of this same file:
Comment #124
jhodgdonAlso given the discussion happening on #2384863: Translation language base field handler should use views field handler, provide unified options I am wondering if we should remove the change to 'language' from this patch and just handle it there.
Comment #125
dawehnerThank you for the review!
Note: this patch just addresses my own review in the train. Trains+
This makes some parts of the diff smaller, some bigger probably.
Comment #127
dawehnerBack to the next fight :)
This is how all things started, dropped the todo.
Note: We just have plugin_id there for config_schema reasons. The actual API doesn't use that kind of configuration. The used plugin ID is defined in the views data.
But right, these places still call out to the node field plugin. Let's change that and hope not everything will break.
Still don't understand why you don't use dreditor :) Good catch btw.
This is one of the places where the abstractions of views don't play well with entity API anymore. ... In views we have a dedicated mechanism to define a linkable field, so we stored the path separated from the title.
This allows us to build generic integrations between fields which have links, so for example we have a dropbutton field, which can be used with things like the node edit link, but at the same time, with a field
configured with a custom path ... which is what this change is doing. No, using fieldapi doesn't have just advantages as you might have thought.
I just dropped that todo. Its not critical to what we want to test, so mocking exactly is not required, IMHO.
Yeah, I guess this was a rebase failure.
Comment #129
dawehnerThere we go.
Comment #131
dawehnerThere we go.
Comment #132
jibranSo what is the future of #2322949: Implement generic entity link view field handlers now.
Comment #133
dawehner@jibran nothing changed in that regard. These entity links will always be there
Comment #134
jibranThanks for the explanation @dawehner. This patch is ready I just have few questions.
Don't we have to update some views as well? Can you please explain why are we dropping this?
What about sticky and promote field? Are we accommodating those? promote? Do we have no view for those in core?
Why we need complete namespace here?
We have a CssSelector in core but nvm.
Comment #135
jhodgdonLooking good!
Can we get rid of the 'node' plugin now (the handler class)?
Comment #136
dawehnerThank you for your review!
Alright, let's adapt the test coverage a bit.
Did you made the research? Afaik there is nothing for the promoted/sticky out there.
Meh, phpstorm refactoring.
Yeah let's not introduce xpath, if possible. Css selectors are so much easier to read.
We can't, because it has a number of subclasses, which are still in use (for example
HistoryUserTimestamp
). As said, this issue is a vulcano, it changes things everywhere.Comment #137
jhodgdonExcellent. I think this is ready, an I believe @jibran's comments have all been addressed. So I am going to go ahead and mark it RTBC; jibran can change if there is a disagreement.
This is a big step forward.
Adding beta eval to summary.
Comment #138
jibranCouldn't agree more. Let's fix this.
Comment #140
dawehnerRerolled.
Comment #141
jibranBack to RTBC
extra white space.
Comment #142
plachI'd like to perform another review, I'll have some time tomorrow after lunch, but don't hold up this on me.
Comment #143
amateescu CreditAttribution: amateescu commentedShouldn't this be 'link_to_entity'?
Empty space :)
I'm wondering what's the reason for this.
Why are we adding a new 'module' key instead of appending 'user' to the previous one?
Missing empty line.
Comment #144
dawehnerThank you for the review!
We want to render the raw URL, not a rendered version of it by default.
Fixed
Comment #145
plachOverall the code looks good, I trust @dawehner's judgement when he says this is enough to fix the critical side of the issue. However we still need to update the issue summary with the current proposed solution.
Comment #146
plachOne more thing: #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality has an impact on the views config entity structures shipped with core, are we planning to write update functions to deal with the changes that will be introduced there?
Comment #147
Gábor HojtsyI think we need to decide which other issues in the family of issues are critical (and D8 upgrade path blocker) given that this issue only covers some of the base fields and the others will still have the same language, formatter and access concerns standing.
Comment #148
alexpottAnd
in NodeViewsData can be removed.
Can remove
from UserViewsData (i think).
Spurious line?
Comment #149
Gábor Hojtsy(BTW that is not required to commit this step IMHO).
Comment #150
jhodgdonI'd also like to do a bit more testing on this before we commit it, upon further thought. And I also think we need another test added. I really don't see any tests that verify that in a multilingual field-based view, the fields are translated properly. We need to add one.
Comment #151
jhodgdon@alexpott - I do not understand your review in #148, items 1 and 2.
Comment #152
jhodgdonSo I tested this out by setting up a multilingual site with two languages and making a Node view... I don't think it's actually all working:
- The Boolean fields are not being translated in output. At least, if I choose an output format like On/Off, True/Fals, Enabled/Disabled, those words are still showing that in English, although if I check my translation status, there are translations for all of these.
- The title field is showing translated, but it links to the base node URL, not the URL of the translation. So I think the Spanish one should link to es/node/1, but it's linking to node/1.
- There were other oddities but I think they're separate issues.
Comment #153
dawehner@jhodgdon
Thank you for testing the functionality, but yeah, even they are potential bugs, let's fix them separate, because the same thing happens if you simply call entity_view() and render the entity translated entity.
Alright, though this is a separate bug, unrelated to this issue #2428103: String formatter should link to its translation.
Alright, let's also add a follow up for that: #2428113: Entity formatters should (maybe?) render UI/Config text that is part of the display using the entity language, not the current interface language
Comment #154
plachIS update :(
Comment #155
jhodgdonOK... I thought this issue was about the ability to translate base fields.
Before this patch, the node title was showing up in the right language anyway, the reason being that the data it took was from the node_field_data table. So this patch isn't fixing that. And you say these other problems are separate issues, which is OK but then what is this patch actually fixing? Can you point to anything that's actually better after this patch, from a user's point of view, and is there a test for it in this patch?
Comment #156
jhodgdonI just feel like every time we fix one of these Views issues for D8MI, we open up more issues than we fix. I'm not seeing a lot of progress. :(
Comment #158
jhodgdonActually I need to test again... realized I am not sure what Language settings I used in the test view I set up.
And to clarify, the node title field was only "translatable" without this patch in the sense that it showed up in the row's entity language no matter what. But we need it to be translatable like other fields: to use the language settings of the view so display in the row language, the page language, etc.
I'm going to give this some more testing...
Comment #159
jhodgdonOK, I did some more testing, centered around the base fields for the Published, Sticky, etc., because those are all Node actually has that are affected by this patch, I think (title and the Boolean fields). I created some translated content, and set these Boolean fields to different values for the English and Spanish translations of the node.
The output was good, aside from the labels for True/False being in the wrong language (#2428113: Entity formatters should (maybe?) render UI/Config text that is part of the display using the entity language, not the current interface language): at least when I had the Display language set to "Views row language", display was picking out the right True/False values at least. (Of course, that would also be true without this patch, because those values would come from the node views data database table fields.)
I still think we need a test for this behavior, but it may be difficult to test until the label language problem is fixed. I'm attaching a test view I made if it is helpful. It is based on the Standard profile with its Article content type that has Tags, and having English and Spanish languages installed. It has 4 displays, for 4 different choices of the display language settings.
I did run into a bug though. For several of the Display Language choices, some of my views rows vanished. For instance, if I chose display language to be a hard-wired English, the Spanish translation row did not show up. Same for Spanish (only Spanish showed up), and UI language (depending on if I put the es/ prefix on the path or not, I got only the English or only the Spanish translation in the output). I only got both translations if I had the display language set to be the view row language. This is odd because I don't have a filter on the view aside from Content Type. I removed this patch and this filtering problem went away (I got both rows displayed in all 4 displays), so it seems to be due to the patch. That's not good.
Anyway, here's the test view I created.
Comment #160
dawehnerThank you for doing that!
Note: I forgot to press the commend form, so I haven't read #159 yet.
Well, IMHO this issue about using the field formatters for rendering these base fields. If the formatters are wrong, this is a different problem. You know this patch is big enough already, if you ask me. This sounds a bit harsh maybe, but I think you cannot calm down a vulcano with one splash of water.
Comment #161
jhodgdonAgreed that this patch is big enough.
Anyway my further testing did uncover a bug introduced by this patch. I have no idea where it's coming from, but the view is definitely being filtered incorrectly (see #159). If we can get that fixed, I'm probably Ok with getting this much in and spewing off multiple other issues. This problem is just Big.
Comment #162
dawehnerThank you @jhodgdon for the detailed test in #159, it really helped to track down the problem.
State of the patch
What this patch does is to remove the base fields from the actual query, but rather get them via the loaded entity, when we render stuff.
So after this the query (beside for the row language display) looks like the following:
The row language display has the following SQL query:
Discussion
This is IMHO a proper explosion of the vulcano ;)
As you see, the
{node_field_data}
is joined less, which let's the result to be just a single node, instead of each of the translations.There are multiple ways to solve it:
{node_field_data}
. This would certainly solve the symptom, but is hacky IMHO.{node}
but rather start from{node_field_data}
and just join into{node}
for the rare case of getting a UUID. I kinda like that solution, but as you could imagine, this could be quite some work.This would save some performance and drop the regression we have in D8 with requiring the join for even the simplest queries.
{node}
and{node_field_data}
... I'm not aware of all the reasons we have that separation, butone was a way to fast count all available nodes. I think it can be implemented pretty fast by using the
default_langcode
flag on {node_field_data}. IMHO this would be the best solution, but for sure probably needs its own dedicated issue and involvement from the various entity people.Comment #163
dawehnerAlright, opened an issue for that problem, as talked on IRC: #2429447: Use data table as views base table, if available.
Comment #164
xjmComment #165
dawehnerJust a reroll to not fall too much behind.
Comment #166
Gábor HojtsyComment #167
dawehnerContinous rebasing.
Comment #168
dawehner*thumbs*
Comment #169
jhodgdonSo... We probably need to verify that the problem I found in #159 does not occur now, and maybe add a test for it? I guess I at least need to go through the same testing I did up around that comment and try it all again. Not today, sorry, but soon...
Comment #170
effulgentsia CreditAttribution: effulgentsia commentedNow that #2429447: Use data table as views base table, if available. landed, is this part of the issue summary still true?
Because if not, why do we still need base fields registered in getViewsData() at all as opposed to expanding the Field UI field handlers to cover all fields? If there's still valid reasons for not doing that, I'm ok with that, just want to know what they are.
Comment #172
dawehnerWell, the statement that the views field handlers do more than the field API formatters is still true, see custom date handling as an example.
Comment #173
dawehnerLet's fix also the failures.
Comment #174
Gábor HojtsyI think this needs a massive issue summary update and a title change to clarify the scope. Both the issue summary and the title make it appear this issue is the scope of #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality so not surprised @effulgentsia is fuzzy about it. Adding the parent.
Comment #175
dawehnerWorked on a better and more up to date issue summary, which clarifies more, hopefully.
Comment #176
jhodgdonTweaking summary a bit and title.
Comment #177
dawehnerReviewed some stuff, but could not find many things.
Comment #178
jhodgdonSo. Could we add a test for the problem I found in #159? The test would:
- Be a WebTestBase.
- Have language, locale, content_translation enabled.
- Add a second language to the site, such as Spanish.
- Turn on content translation for a node type.
- Create a node and translate it into Spanish. Make the base node have the "sticky" field set to TRUE and the translation have it FALSE.
- Check the displays of a view to verify that they display both translations, and in the correct language.
The view would need to have the following features:
- Uses fields.
- Displays the node title, no link necessary.
- Displays the Sticky field. Use the 1/0 option for the sticky field formatter, and a label of "Sticky".
There would be 4 displays that create pages, with 4 different display language settings:
a) Language of the row - should show titles translated properly, and "Sticky: 1" for English, "Sticky: 0" for Spanish node.
b) Use English - should show the English title for both nodes, and "Sticky: 1" for English, "Sticky: 0" for Spanish node.
c) Use Spanish - should show the Spanish title for both nodes, and "Sticky: 1" for English, "Sticky: 0" for Spanish node.
d) Use page language. Should look like (b) if you display the base URL, and (c) if you go to es/[path].
I think that adding this test would be good, to ensure that the bug I found with this patch is fixed now that we're using the data tables in views, and also to ensure that it doesn't recurr, and also to ensure that the field values are being translated. There are existing multilingual views tests that illustrate how to set up the module... shouldn't take much time to write this test?
Comment #179
dawehnerYeah I think we should ensure that this really works.
Here is a small cleanup which we could do.
Comment #180
dawehnerRerolled
Comment #182
dawehnerAnother proper reroll.
Comment #183
dawehnerComment #184
plachSkimming through the patch I found a few more (very) minor issues:
"Nitpick: Afaik we use more === these days." (cit.) ;)
Weird indentation: can we indent all arguments, and use two indentation levels for the array items of the second argument?
Wrong indentation.
Doing some more testing now.
Comment #185
dawehnerAdditional follow up: #2449627: How to deal with bypassing entity API for custom storage examples: forum_index, tracker etc.
Comment #186
dawehnerWent through all views.view.*.yml files
Comment #188
dawehnerFix at least one schema failure.
Comment #189
dawehnerFix at least one schema failure.
Comment #191
dawehnerMore schema fixes.
Comment #192
dawehnerDid some really basic profiling:
50 nodes on a page.
Table view with the following fields: title, nid, vid, status, sticky, frontpage, uuid ...
No xdebug
No optimized autoloader
Result: basically factor 2 slower.
HEAD: https://blackfire.io/profiles/6b8b8432-fbf4-4688-8490-b43f30d20b9b/graph
PATCH: https://blackfire.io/profiles/06de03e4-0d7b-4a8b-839a-5dae6207fddc/graph
Comparision: https://blackfire.io/profiles/compare/91160890-7703-4dd5-ab04-3405908b0c...
Comment #193
Wim LeersDoes that mean this is blocked on #1867518: Leverage entityDisplay to provide fast rendering for fields, to at least prove there that we can regain that lost performance?
Comment #194
Fabianx CreditAttribution: Fabianx commentedFactor 2, ouch that is hard ... I think yes, we might need to tackle #1867518: Leverage entityDisplay to provide fast rendering for fields first ...
Comment #195
jhodgdonThe performance hit is bad, but this issue is Critical because it has security implications (field/entity permissions), and also because it's needed for multilingual sites to make the fields translatable. So we don't really have a choice on whether or not to do this, and I do not think it is appropriate to block this critical bug on making it more efficient. That can (I think) be done afterwards?
Comment #196
Wim Leers#195: I'd personally be fine with that approach. But it'd be great if we could update the IS to explain why this is a security bug in D8 core, but not in D7 Views? Doesn't the same problem exist there? Why not?
Explaining that would go a long way to supporting #195.
Comment #197
dawehnerYeah, let's do that.
Ha, I never could figure out why exactly it looks wrong. Fixed that.
PS:
More follow ups:
#2450151: Don't try to render all fields (including hidden ones) for single entity display
#2450153: Add a default_formatter to UUIDs fields
Worked on a test according to comment #178 as well
Comment #198
dawehnerAdapted the issue summary regarding #196, feel free to improve it.
Comment #199
dawehnerYou should simply not trust tests without running them.
Comment #201
jhodgdonThe test looks good to me! Also the issue summary update.
Comment #202
plachComment #203
dawehnerSome small bits.
Comment #204
plachLooks good to me too! Some other nitpicks:
Missing PHP doc
We should pass
new Language('es')
as an option to::drupalGet()
.Comment #205
dawehnerThank you for your review!
Comment #206
plachThis looks ready to me.
@Wim: I'd like to RTBC this, does the issue summary look compelling now?
Comment #207
plach@Wim:
Feel free to set back to needs review / need work :)
Btw, not sure whether we need a change record, since we have "only" config changes here.
Comment #210
Fabianx CreditAttribution: Fabianx commentedWe need a critical follow-up to address the performance regression introduced here.
I like the patch and am okay with RTBC, but we should really have a plan of how to address the performance problems introduced here.
We do it later has not served us well in the past and because we discussed this in IRC there are several things we could do:
1. We can now render cache the properties by introducing field based render caching (this works well in D7 and e.g. Dave Reid implemented that for my render_cache module)
2. We can cache the rendered views rows, similar to plach's views_row_cache (https://www.drupal.org/project/views_row_cache)
3. We can make the entity display issue critical, which allows caching and a performance boost.
However - just the standard views_output cache is not gonna cut it.
Because we have three feasible possibilities on solving the performance regression and cacheability gets improved and entity display issue easier, I am giving this a RTBC + 1 and am okay with the Performance regression (please do not remove the tag - I want to track that from now on a little more).
This will also profit from a change record so people know what to expect now as its different from D7, but leaving to catch to see if a CR is necessary.
So things todo from my POV:
- Critical follow-up to fix the regression (at this stage we don't need more performance regressions) -- can just put my 3 possibilities in there, make it a meta and whoever is implemented wins.
- Change record from D7
Besides that:
Great work!
Comment #211
Wim Leers+1 to everything Fabian said :)
(With the exception of his first option for fixing the performance regression: I doubt that's effective. That still leaves options 2 and 3 though.)
Awesome work!
Comment #212
plachThanks guys, I just wanted to outline that I promoted #1867518: Leverage entityDisplay to provide fast rendering for fields to critical as soon as I started working on it.
Comment #213
catchOpened #2450897: Cache Field views row output. Agreed with #210, not a lot of choice here and we have viable ways to fix it.
Haven't reviewed the patch in full yet so not committing just now.
Comment #214
joshtaylor CreditAttribution: joshtaylor commentedStraight reroll.
Comment #216
dawehnerLet's fix things for now, but clean up stuff much better in #2451679: Validate cache contexts (+ cache contexts in some views plugins wrong)
Comment #217
plachComment #219
catchOK reviewed the patch again and I think this is fine. The main performance issue is already critical, and we have other issues around views rendering and caching which this will simplify. Good to see a lot of things being brought into line.
Committed/pushed to 8.0.x, thanks!
Comment #220
dawehnerThank you a lot!!
Comment #221
YesCT CreditAttribution: YesCT commentedthis issue still has the needs change record tag.
I made a draft: https://www.drupal.org/node/2452797
Please correct it. :) and then remove the tag.
Also, I wonder if that should be the change record for #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality
Comment #222
Gábor HojtsyUpdated and published https://www.drupal.org/node/2452797, thanks for the draft!
Thanks all, especially @dawehner for the incredible work here! Amazing!
Comment #223
jbrown CreditAttribution: jbrown commentedThanks everyone for working so hard to fix this issue!
Comment #224
yched CreditAttribution: yched commentedImpressive work, @dawehner !
Comment #225
BerdirSomehow this caused #2453551: TranslationLanguageRenderer tries to add langcode field to the view for entity types that have no langcode, I guess because I'm now using the field plugin for an entity type and didn't before. Please help :)
Comment #226
xjmSO awesome.
Comment #228
apaderno