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:

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

Reference: https://www.drupal.org/core/beta-changes
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.
CommentFileSizeAuthor
#216 interdiff.txt2.44 KBdawehner
#216 2342045-216.patch121.06 KBdawehner
#214 2342045-214.patch125.1 KBjoshtaylor
#205 interdiff.txt1.26 KBdawehner
#205 2342045-205.patch119.65 KBdawehner
#203 interdiff.txt1.93 KBdawehner
#203 2342045-202.patch119.38 KBdawehner
#199 interdiff.txt2.52 KBdawehner
#199 2342045-199.patch119.88 KBdawehner
#197 interdiff.txt13.8 KBdawehner
#197 2342045-194.patch119.88 KBdawehner
#191 interdiff.txt7 KBdawehner
#191 2342045-191.patch109.29 KBdawehner
#188 interdiff.txt508 bytesdawehner
#188 2342045-188.patch106.32 KBdawehner
#186 interdiff.txt43.83 KBdawehner
#186 2342045-186.patch106.3 KBdawehner
#182 2342045-182.patch65.2 KBdawehner
#180 2342045-180.patch65.21 KBdawehner
#179 interdiff.txt773 bytesdawehner
#179 2342045-179.patch65.21 KBdawehner
#177 interdiff.txt617 bytesdawehner
#177 2342045-177.patch64.45 KBdawehner
#173 interdiff.txt5.85 KBdawehner
#173 2342045-173.patch64.75 KBdawehner
#168 2342045-168.patch63.95 KBdawehner
#167 2342045-167.patch64.26 KBdawehner
#165 2342045-165.patch64.26 KBdawehner
#159 views.view_.test.yml.txt12.1 KBjhodgdon
#153 interdiff.txt3.59 KBdawehner
#153 2342045-153.patch64.26 KBdawehner
#152 preview.png13.05 KBjhodgdon
#144 interdiff.txt2.28 KBdawehner
#144 2342045-144.patch60.88 KBdawehner
#140 2342045-140.patch61.03 KBdawehner
#136 interdiff.txt6.49 KBdawehner
#136 2342045-135.patch61.22 KBdawehner
#131 interdiff.txt1.27 KBdawehner
#131 2342045-131.patch60.91 KBdawehner
#129 interdiff.txt295 bytesdawehner
#129 2342045-129.patch60.77 KBdawehner
#127 interdiff.txt2.73 KBdawehner
#127 2342045-126.patch60.74 KBdawehner
#125 interdiff-2342045.txt14.46 KBdawehner
#125 2342045-123.patch60.44 KBdawehner
#122 interdiff.txt3.95 KBdawehner
#122 2342045-122.patch60.89 KBdawehner
#118 interdiff.txt776 bytesdawehner
#118 2342045-118.patch59.8 KBdawehner
#116 interdiff.txt15.1 KBdawehner
#116 2342045-114.patch59.8 KBdawehner
#110 interdiff.txt13.06 KBdawehner
#110 2342045-110.patch51.51 KBdawehner
#108 interdiff.txt2.86 KBGábor Hojtsy
#108 2342045-108.patch40.96 KBGábor Hojtsy
#106 interdiff.txt3.45 KBdawehner
#106 2342045-106.patch41.13 KBdawehner
#104 2342045-104.patch40.76 KBdawehner
#100 interdiff.txt1.27 KBdawehner
#100 2342045-100.patch48.08 KBdawehner
#100 interdiff.txt1.27 KBdawehner
#100 2342045-100.patch48.08 KBdawehner
#91 interdiff.txt6.95 KBdawehner
#91 2342045-91.patch48.43 KBdawehner
#87 interdiff.txt2.23 KBdawehner
#87 2342045-87.patch43.62 KBdawehner
#85 interdiff.txt5.71 KBdawehner
#85 2342045-85.patch43.28 KBdawehner
#83 2342045-83.patch41.35 KBdawehner
#75 2342045-75.patch80.3 KBdawehner
#75 interdiff.txt1.35 KBdawehner
#73 interdiff.txt9.07 KBdawehner
#73 2342045-73.patch95.33 KBdawehner
#71 interdiff.txt18.95 KBdawehner
#71 2342045-71.patch94.85 KBdawehner
#69 2342045-69.patch64.39 KBdawehner
#64 2342045-64.patch64.91 KBdawehner
#64 interdiff.txt2.73 KBdawehner
#57 2342045-57.patch54.46 KBdawehner
#57 interdiff.txt1.27 KBdawehner
#54 2342045-54.patch53.25 KBdawehner
#54 interdiff.txt16.02 KBdawehner
#52 interdiff.txt4.23 KBdawehner
#52 2342045-52.patch46.33 KBdawehner
#50 interdiff.txt11.02 KBdawehner
#50 2342045-50.patch46.26 KBdawehner
#48 interdiff.txt13.58 KBdawehner
#48 2342045-48.patch36.66 KBdawehner
#45 interdiff.txt3.05 KBdawehner
#45 2342045-45.patch35.81 KBdawehner
#40 views-formatters-base-fields-2342045.40.patch42.56 KBlarowlan
#40 interdiff.txt6.39 KBlarowlan
#35 2342045-35.patch36.16 KBdawehner
#33 interdiff.txt8.45 KBdawehner
#33 2342045-33.patch33.25 KBdawehner
#26 interdiff.txt13.12 KBdawehner
#25 2280961-25.patch26.21 KBdawehner
#23 result.png6.61 KBdawehner
#22 2342045-22.patch15.86 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jbrown’s picture

Title: Formatter cannot be selected for base fields » Formatter cannot be selected in views for base fields
jbrown’s picture

Issue summary: View changes
jbrown’s picture

Priority: Normal » Critical

I'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.

amateescu’s picture

Priority: Critical » Normal

Please take a quick look at https://www.drupal.org/node/45111 to see when the critical priority should be used :)

Gábor Hojtsy’s picture

Priority: Normal » Major
Issue tags: +VDC

#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.

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-content, +sprint
jhodgdon’s picture

The 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.

Berdir’s picture

Assigned: Unassigned » dawehner

@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.

Gábor Hojtsy’s picture

@jhodgdon: those base field handlers have equivalents mostly as configurable field handlers too, no? Except markup and standard maybe?

dawehner’s picture

@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.

  • How do we implement "raw" data access, for stuff like views data export, should we actually support it?
  • Do we want to bring over all functionality of the fieldapi field handler? (skip itemsw, reverse ...)
  • How does formatters interact with rewrite (note: rewrite is kinda broken anyway (TODO fill in the existing issue)), I guess we want to expose
    the raw data and the formatted data as before?
  • How do we ensure that we don't run into a crazy performance decline
  • We have to ensure that groupby/aggregate functionality is still working for these base fields (note: this is a tricky thing in general)
  • Maybe more ...

In other words, having some proper strategy first would be really helpful.

dawehner’s picture

Just to be clear, every step towards entity query (which is one is also kinda one) is a good idea!

jhodgdon’s picture

RE #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.

yched’s picture

Right, 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 :-/

jhodgdon’s picture

So... 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?

Gábor Hojtsy’s picture

Entity 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).

jhodgdon’s picture

RE #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?

Gábor Hojtsy’s picture

Yeah 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 :)

jhodgdon’s picture

Assigned: dawehner » Unassigned
Issue tags: +Needs issue summary update

We can always try assigning it to @alexpott for feedback... might need an issue summary update first though, since it has gone beyond just formatters.

jhodgdon’s picture

Issue summary: View changes
Priority: Major » Critical
Related issues: +#1867518: Leverage entityDisplay to provide fast rendering for fields

Bump. 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?

jhodgdon’s picture

Title: Formatter cannot be selected in views for base fields » Formatter and language cannot be selected in views for base fields
dawehner’s picture

Issue summary: View changes
dawehner’s picture

Status: Active » Needs review
FileSize
15.86 KB

Worked a bit on that.

The approach was basically:

To be honest I'm quite suprised.

dawehner’s picture

FileSize
6.61 KB

Here is the missing screenshot.

Status: Needs review » Needs work

The last submitted patch, 22: 2342045-22.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
26.21 KB

Fixed 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.

dawehner’s picture

FileSize
13.12 KB

Forgot the interdiff ...

Status: Needs review » Needs work

The last submitted patch, 25: 2280961-25.patch, failed testing.

jhodgdon’s picture

I 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?

+++ b/core/lib/Drupal/Core/Field/BaseFieldDefinition.php
@@ -92,7 +92,6 @@ public static function createFromFieldStorageDefinition(FieldStorageDefinitionIn
       ->setName($definition->getName())
       ->setProvider($definition->getProvider())
       ->setQueryable($definition->isQueryable())
-      ->setRequired($definition->isRequired())

Another minor thing with the patch, in FieldPluginBase:

-  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityManagerInterface $entity_manager, FormatterPluginManager $formatter_plugin_manager, LanguageManagerInterface $language_manager) {
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityManagerInterface $entity_manager, FormatterPluginManager $formatter_plugin_manager, FieldTypePluginManagerInterface $field_type_plugin_manager, LanguageManagerInterface $language_manager) {

Needs a corresponding addition to the doc block for the new @param?

larowlan’s picture

dawehner’s picture

I 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.

Sure, there was always a good reason why this was a special fish. I'm still not sure what approach would be sane here.

Also, in the patch, this looks like a mistake, or at best something that isn't part of this issue?

Well, FieldStorageDefinitionInterface doesn't have this method, so it failed my unit test.

Needs a corresponding addition to the doc block for the new @param?

Sure sure ...

yched’s picture

Right, 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 ?

dawehner’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
33.25 KB
8.45 KB

Merged in the other patch for now, to see how many failures are left.

Status: Needs review » Needs work

The last submitted patch, 33: 2342045-33.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
36.16 KB

Just a reroll for now.

Status: Needs review » Needs work

The last submitted patch, 35: 2342045-35.patch, failed testing.

catch’s picture

Title: [PP-1] Formatter and language cannot be selected in views for base fields » Formatter and language cannot be selected in views for base fields
Status: Postponed » Needs work
Issue tags: +blocker.

Posted on the wrong issue, fixing metadata.

Marked #2385443: Test that base entity fields on views respect field level access postponed on this.

tim.plunkett’s picture

Issue tags: -blocker. +blocker
larowlan’s picture

Issue tags: +CriticalADay

poking this along

larowlan’s picture

Status: Needs work » Needs review
FileSize
6.39 KB
42.56 KB

this change resulted in the {user_roles}.roles_target_id being renamed to {user_roles}.roles

diff --git a/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
index f992081..e9dbf72 100644
--- a/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
+++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
@@ -323,7 +323,15 @@ protected function generateFieldTableName(FieldStorageDefinitionInterface $stora
    * {@inheritdoc}
    */
   public function getFieldColumnName(FieldStorageDefinitionInterface $storage_definition, $column) {
-    return in_array($column, $this->getReservedColumns()) ? $column : $storage_definition->getName() . '_' . $column;
+    if (in_array($column, $this->getReservedColumns())) {
+      return $column;
+    }
+
+    if ($storage_definition->isBaseField() && count($storage_definition->getColumns()) === 1) {
+      return $storage_definition->getName();
+    }
+
+    return $storage_definition->getName() . '_' . $column;
   }

 }

Which meant SessionHandler::read() had an invalid SQL query.
Not sure if this is the right fix, might blow up spectacularly - but installs locally.

Status: Needs review » Needs work

The last submitted patch, 40: views-formatters-base-fields-2342045.40.patch, failed testing.

Berdir’s picture

Did 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?

dawehner’s picture

Opened up #2392209: DefaultTableMapping::getFieldColumName is broken for base tables ... its a blocker, but its not a blocker to continue work here.

Gábor Hojtsy’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
35.81 KB
3.05 KB

Let's ask the bot again.

Status: Needs review » Needs work

The last submitted patch, 45: 2342045-45.patch, failed testing.

plach’s picture

Issue tags: +Ghent DA sprint
dawehner’s picture

Status: Needs work » Needs review
FileSize
36.66 KB
13.58 KB

Some more work.

AAAAAAAAAAh

Status: Needs review » Needs work

The last submitted patch, 48: 2342045-48.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
46.26 KB
11.02 KB

Fixed 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.

Status: Needs review » Needs work

The last submitted patch, 50: 2342045-50.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
46.33 KB
4.23 KB

Okay, let's install again ...

Status: Needs review » Needs work

The last submitted patch, 52: 2342045-52.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
16.02 KB
53.25 KB

Some 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.

Status: Needs review » Needs work

The last submitted patch, 54: 2342045-54.patch, failed testing.

jhodgdon’s picture

Splitting 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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.27 KB
54.46 KB

Kind of a reroll, added another child issue. It is a vulcano of issues if we want to fix it properly.

Status: Needs review » Needs work

The last submitted patch, 57: 2342045-57.patch, failed testing.

Status: Needs work » Needs review

likin queued 57: 2342045-57.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 57: 2342045-57.patch, failed testing.

jhodgdon’s picture

Definitely a volcano. :(

dawehner’s picture

Note: Currently this patch doesn't install because of the following message:

You are about to DROP all tables in your 'dev_d8' database. Do you want to continue? (y/n): y
Starting Drupal installation. This takes a while. Consider using the --notify global option.                    [ok]
exception 'Drupal\Component\Plugin\Exception\PluginNotFoundException' with message 'The "published-notpublished"[error]
plugin does not exist.' in /var/www/d8/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php:57
Stack trace:

... 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.

dawehner’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.73 KB
64.91 KB

Merged 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.

Status: Needs review » Needs work

The last submitted patch, 64: 2342045-64.patch, failed testing.

dawehner’s picture

Status: Needs work » Postponed

In 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.

dawehner’s picture

Issue tags: +entity storage

We need more tags

slashrsm’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
64.39 KB

Alright, let's see how horrible the reroll was.

Status: Needs review » Needs work

The last submitted patch, 69: 2342045-69.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
94.85 KB
18.95 KB

The 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.

Status: Needs review » Needs work

The last submitted patch, 71: 2342045-71.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
95.33 KB
9.07 KB

Again some more work.

Status: Needs review » Needs work

The last submitted patch, 73: 2342045-73.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.35 KB
80.3 KB

Just a reroll and one fix.

Status: Needs review » Needs work

The last submitted patch, 75: 2342045-75.patch, failed testing.

webchick’s picture

Issue tags: +D8 upgrade path

Since this is security-related, tagging as D8 upgrade path.

dawehner’s picture

Gábor Hojtsy’s picture

Status: Postponed » Needs work

Status: Needs work » Needs review

Gábor Hojtsy queued 75: 2342045-75.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 75: 2342045-75.patch, failed testing.

dawehner’s picture

Working on a reroll for now.

dawehner’s picture

Status: Needs work » Needs review
FileSize
41.35 KB

.

Status: Needs review » Needs work

The last submitted patch, 83: 2342045-83.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +needs profiling
FileSize
43.28 KB
5.71 KB

Let'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.

Status: Needs review » Needs work

The last submitted patch, 85: 2342045-85.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
43.62 KB
2.23 KB

A few test failures.

Gábor Hojtsy’s picture

Based 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!

Status: Needs review » Needs work

The last submitted patch, 87: 2342045-87.patch, failed testing.

Gábor Hojtsy’s picture

Based 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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
48.43 KB
6.95 KB

Some more "funny" debugging.

Status: Needs review » Needs work

The last submitted patch, 91: 2342045-91.patch, failed testing.

Gábor Hojtsy’s picture

Title: Formatter and language cannot be selected in views for base fields » Formatter cannot be selected in views for base fields, access is not checked on them

This 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.

jhodgdon’s picture

Title: Formatter cannot be selected in views for base fields, access is not checked on them » Formatter and display language cannot be selected in views for base fields, access is not checked on them

Actually the title was referring to "you can't choose what language to display base fields in", not "the language fields have a problem". ;)

Gábor Hojtsy’s picture

@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.

jhodgdon’s picture

I 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?

jhodgdon’s picture

Title: Formatter and display language cannot be selected in views for base fields, access is not checked on them » Views base fields need to use same rendering as Field UI fields, for formatting, acess checking, and translation consistency

Oh 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?

jhodgdon’s picture

Title: Views base fields need to use same rendering as Field UI fields, for formatting, acess checking, and translation consistency » Views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency

fix typo.

plach’s picture

Quickly skimming through this to get some familiarity with the code. I noticed a few code style issues:

  1. +++ b/core/modules/field/src/Tests/Boolean/BooleanFormatterTest.php
    @@ -0,0 +1,144 @@
    \ No newline at end of file
    

    :)

  2. +++ b/core/modules/user/user.info.yml
    @@ -6,3 +6,5 @@ version: VERSION
    \ No newline at end of file
    

    :))

  3. +++ b/core/modules/views/src/Plugin/views/field/Field.php
    @@ -365,9 +365,16 @@ protected function defineOptions() {
    +    elseif (isset($field_type['default_formatter'])) {
    +
    +      $options['type'] = ['default' => $field_type['default_formatter']];
    

    Surplus blank line ;)

dawehner’s picture

Status: Needs work » Needs review
FileSize
48.08 KB
1.27 KB
48.08 KB
1.27 KB

Rerolled, fixed the feedback from #99, thank you @plach!

Status: Needs review » Needs work

The last submitted patch, 100: 2342045-100.patch, failed testing.

Gábor Hojtsy’s picture

#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.

pcambra’s picture

Some things I've found:

  1. +++ b/core/modules/views/src/Tests/GroupRowsTest.php
    @@ -0,0 +1,98 @@
    +  protected $node_type;
    +
    +  protected $field_name;
    +
    +  protected $field_storage;
    +
    +  protected $field;
    

    This might not be needed as protected but just variables in the setUp

  2. +++ b/core/modules/views/src/Tests/GroupRowsTest.php
    @@ -0,0 +1,98 @@
    +      'bundle' => $this->node_type->type,
    

    node_type->id()

  3. +++ b/core/modules/views/src/Tests/GroupRowsTest.php
    @@ -0,0 +1,98 @@
    + * Definition of Drupal\views\Tests\GroupRowsTest.
    

    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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
40.76 KB

Just a reroll for now.

Status: Needs review » Needs work

The last submitted patch, 104: 2342045-104.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
41.13 KB
3.45 KB

For now just a reroll + fixing of the existing schema issues (hopefully)

Status: Needs review » Needs work

The last submitted patch, 106: 2342045-106.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
40.96 KB
2.86 KB

Fixing the rest of the schema fails I think.

Status: Needs review » Needs work

The last submitted patch, 108: 2342045-108.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
51.51 KB
13.06 KB

Thank you for that!

Here is a WIP from adding actual revision support back, there are small bugs here and there.

Status: Needs review » Needs work

The last submitted patch, 110: 2342045-110.patch, failed testing.

jhodgdon’s picture

This is so close!

A few cosmetic glitches I noticed in the patch, and two real questions at the end:

a)

+++ b/core/modules/node/src/Plugin/views/wizard/Node.php
@@ -88,6 +88,7 @@ protected function defaultDisplayOptions() {
     $display_options['fields']['title']['table'] = 'node_field_data';
     $display_options['fields']['title']['field'] = 'title';
     $display_options['fields']['title']['entity_type'] = 'node';
+    $display_options['fields']['title']['entity_type'] = 'node';

Duplicated line here?

b)

+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestRev.php
@@ -23,6 +23,7 @@
  *       "default" = "Drupal\entity_test\EntityTestForm",
  *       "delete" = "Drupal\entity_test\EntityTestDeleteForm"
  *     },
+ *    "view_builder" = "Drupal\entity_test\EntityTestViewBuilder",
  *     "translation" = "Drupal\content_translation\ContentTranslationHandler",

I think this needs one more space of indentation in the added line?

c)

+++ b/core/modules/user/config/install/views.view.user_admin_people.yml
@@ -292,11 +292,12 @@ display:
           hide_empty: false
           empty_zero: false
           hide_alter_empty: true
-          type: active-blocked
-          type_custom_true: ''
-          type_custom_false: ''
-          not: false
-          plugin_id: boolean
+          plugin_id: field
+          type: boolean
+          settings:
+            format: custom
+            format_custom_true: 'Active'
+            format_custom_false: 'False'

I think the custom_false value here should be 'Blocked', not 'False'? Just guessing but...

d)

+++ b/core/modules/views/src/Plugin/views/query/Sql.php
@@ -1465,8 +1465,10 @@ function execute(ViewExecutable $view) {
    * If the entity belongs to the base table, then it gets stored in
    * $result->_entity. Otherwise, it gets stored in
    * $result->_relationship_entities[$relationship_id];
+   *
+   * @param \Drupal\views\ResultRow[] $results
    */
-  function loadEntities(&$results) {
+  public function loadEntities(&$results) {

The @param needs a documentation line.

e) GroupRowsTest

+
+  protected $node_type;
+
+  protected $field_name;
+
+  protected $field_storage;
+
+  protected $field;

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

dawehner’s picture

This is so close!

Mhh you kinda see something different than me probably :)

I think the custom_false value here should be 'Blocked', not 'False'? Just guessing but...

Yeah you are right here.

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.

Well, as long we haven't done #2399211: Support all options from views fields in DateTime formatters switching over is not that easy :)

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

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.

jhodgdon’s picture

Regarding 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.

jhodgdon’s picture

One 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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
59.8 KB
15.1 KB
  • Fixed the small points in the good review of @jhodgdon #112
  • Had some issues with revisions but thanks to talking with bojanz it could be solved in a good way.

Answer to #115

numeric

- Used in FileViewsData for fields on usage table: id, count

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.

- Used in CommentViewsData for field comment_count, last_comment_uid

Note:For comment_entity_statistics we can apply the same argument as above.

standard

- Used in FileViewsData for fields on usage table: module, type
- Used in CommentViewsData for fields field_name (on two tables), entity_type

Same point...

- Used in UserViewsData

Opened a dedicated follow up for the signature bit: #2425193: Convert the signature field in user_field_data to the ordinary views field

Status: Needs review » Needs work

The last submitted patch, 116: 2342045-114.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
59.8 KB
776 bytes

This fix could be simple

Status: Needs review » Needs work

The last submitted patch, 118: 2342045-118.patch, failed testing.

jhodgdon’s picture

Ah, good points about those non-base tables. Will update the meta issue.

jhodgdon’s picture

Interdiffs above look good, by the way!

dawehner’s picture

Status: Needs work » Needs review
FileSize
60.89 KB
3.95 KB

Perfect, less workarounds are needed.

jhodgdon’s picture

Status: Needs review » Needs work

OMG 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:

+    // @fixme Move this into the generic views data.

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:

           table: node
           field: nid
           relationship: nid
-          plugin_id: node
+          plugin_id: field
           entity_type: node
           entity_field: nid

This seems inconsistent. I think we want to change this line near the top of NodeViewsData:

    $data['node']['nid']['field']['id'] = 'node';

to use 'field'?

This other change in a views config looks wrong:

       arguments:
@@ -49,7 +49,7 @@ display:
           id: nid
           table: node_revision
           field: nid
-          plugin_id: node_nid
+          plugin_id: field

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:

       case 'uri':
-        $views_field['field']['id'] = 'url';
+        // Let's render URIs as URIs by default, not links.
+        $views_field['field']['id'] = 'field';
+        $views_field['field']['default_formatter'] = 'string';
+

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:

+++ b/core/modules/views/src/Tests/Plugin/StyleOpmlTest.php
@@ -58,6 +58,7 @@ public function testOpmlOutput() {
 
     $this->drupalGet('test-feed-opml-style');
     $outline = $this->xpath('//outline[1]');
+    debug($feed->getUrl());

e) This change to the link to entity/node setting looks wrong:

core/modules/views/tests/modules/views_test_config/test_views/views.view.test_dropbutton.yml
-          link_to_node: true
+          type: string
+          settings:
+            link_to_entity: false

f) Why is this documentation being removed?

+++ b/core/modules/views/tests/src/Unit/Plugin/field/FieldTest.php
@@ -27,18 +27,8 @@ class FieldTest extends UnitTestCase {
    */
   protected $entityManager;
 
-  /**
-   * The mocked formatter plugin manager.
-   *
-   * @var \Drupal\Core\Field\FormatterPluginManager|\PHPUnit_Framework_MockObject_MockObject
-   */
   protected $formatterPluginManager;

(and others)

g) In FieldTest again:

+      // @todo replace the second anything() with FALSE.
+      ->with('view', $this->anything(), $account, NULL, $this->anything())
 

Not sure what this @todo is?

h) Still there

   *
-   * @covers ::clickSort
    */
   public function testClickSortWithOutConfiguredColumn($order) {

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?

  /**
-   * Returns a mocked base field storage object.
-   *
    * @return \Drupal\Core\Field\FieldStorageDefinitionInterface|\PHPUnit_Framework_MockObject_MockObject
    */
   protected function getBaseFieldStorage() {

j) Line added to end of this same file:

 }
+
jhodgdon’s picture

Also 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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
60.44 KB
14.46 KB

Thank 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.

Status: Needs review » Needs work

The last submitted patch, 125: 2342045-123.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
60.74 KB
2.73 KB

Back to the next fight :)

This probably either needs to be removed, done now, or replaced with a @todo (not @fixme) and a follow-up issue.

This is how all things started, dropped the todo.

n 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.

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.

This other change in a views config looks wrong:

Still don't understand why you don't use dreditor :) Good catch btw.

This change to the link to entity/node setting looks wrong:

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.

Not sure what this @todo is?

I just dropped that todo. Its not critical to what we want to test, so mocking exactly is not required, IMHO.

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.

Yeah, I guess this was a rebase failure.

Status: Needs review » Needs work

The last submitted patch, 127: 2342045-126.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
60.77 KB
295 bytes

There we go.

Status: Needs review » Needs work

The last submitted patch, 129: 2342045-129.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
60.91 KB
1.27 KB

There we go.

jibran’s picture

+++ b/core/modules/node/config/install/views.view.content.yml
@@ -163,10 +163,12 @@ display:
-          link_to_node: true
-          plugin_id: node
...
+          type: string
+          settings:
+            link_to_entity: true
+          plugin_id: field

So what is the future of #2322949: Implement generic entity link view field handlers now.

dawehner’s picture

@jibran nothing changed in that regard. These entity links will always be there

jibran’s picture

Thanks for the explanation @dawehner. This patch is ready I just have few questions.

  1. +++ b/core/modules/block_content/src/Tests/Views/RevisionRelationshipsTest.php
    @@ -59,7 +59,6 @@ public function testBlockContentRevisionRelationship() {
    -      'block_content_revision_id' => 'block_content_revision_id',
    
    @@ -69,12 +68,10 @@ public function testBlockContentRevisionRelationship() {
    -        'block_content_revision_id' => '1',
    ...
    -        'block_content_revision_id' => '1',
    
    @@ -86,7 +83,6 @@ public function testBlockContentRevisionRelationship() {
    -        'block_content_revision_id' => '1',
    
    +++ b/core/modules/node/src/Tests/Views/RevisionRelationshipsTest.php
    @@ -48,7 +48,6 @@ public function testNodeRevisionRelationship() {
    -      'node_revision_nid' => 'node_revision_nid',
    
    @@ -58,12 +57,10 @@ public function testNodeRevisionRelationship() {
    -        'node_revision_nid' => '1',
    ...
    -        'node_revision_nid' => '1',
    
    @@ -75,7 +72,6 @@ public function testNodeRevisionRelationship() {
    -        'node_revision_nid' => '1',
    

    Don't we have to update some views as well? Can you please explain why are we dropping this?

  2. +++ b/core/modules/node/config/install/views.view.content.yml
    @@ -222,11 +224,12 @@ display:
    +          type: boolean
    +          settings:
    +            format: custom
    +            format_custom_true: 'Published'
    +            format_custom_false: 'Unpublished'
    +          plugin_id: field
    

    What about sticky and promote field? Are we accommodating those? promote? Do we have no view for those in core?

  3. +++ b/core/modules/options/src/Plugin/views/argument/NumberListField.php
    @@ -26,7 +26,7 @@
    -  use FieldAPIHandlerTrait;
    +  use \Drupal\views\FieldAPIHandlerTrait;
    
    +++ b/core/modules/options/src/Plugin/views/argument/StringListField.php
    @@ -25,7 +25,7 @@
    -  use FieldAPIHandlerTrait;
    +  use \Drupal\views\FieldAPIHandlerTrait;
    
    +++ b/core/modules/options/src/Plugin/views/filter/ListField.php
    @@ -21,7 +21,7 @@
    -  use FieldAPIHandlerTrait;
    +  use \Drupal\views\FieldAPIHandlerTrait;
    
    +++ b/core/modules/views/src/Plugin/views/field/Field.php
    @@ -41,7 +41,7 @@
    -  use FieldAPIHandlerTrait;
    +  use \Drupal\views\FieldAPIHandlerTrait;
    

    Why we need complete namespace here?

  4. +++ b/core/modules/views/src/Tests/Handler/FieldGroupRowsWebTest.php
    @@ -0,0 +1,124 @@
    +    $result = $this->xpath('//div[contains(@class, "views-field-field-views-testing-group-")]/div');
    ...
    +    $result = $this->xpath('//div[contains(@class, "views-field-field-views-testing-group-")]/div');
    

    We have a CssSelector in core but nvm.

jhodgdon’s picture

Looking good!

Can we get rid of the 'node' plugin now (the handler class)?

dawehner’s picture

FileSize
61.22 KB
6.49 KB

Thank you for your review!

Don't we have to update some views as well? Can you please explain why are we dropping this?

Alright, let's adapt the test coverage a bit.

What about sticky and promote field? Are we accommodating those? promote? Do we have no view for those in core?

Did you made the research? Afaik there is nothing for the promoted/sticky out there.

Why we need complete namespace here?

Meh, phpstorm refactoring.

We have a CssSelector in core but nvm.

Yeah let's not introduce xpath, if possible. Css selectors are so much easier to read.

Can we get rid of the 'node' plugin now (the handler class)?

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.

jhodgdon’s picture

Issue summary: View changes

Excellent. 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.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

As said, this issue is a vulcano, it changes things everywhere.

Couldn't agree more. Let's fix this.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 136: 2342045-135.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
61.03 KB

Rerolled.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

+++ b/core/modules/views/config/schema/views.field.schema.yml
@@ -182,3 +182,50 @@ views.field.language:
+  ¶

extra white space.

plach’s picture

I'd like to perform another review, I'll have some time tomorrow after lunch, but don't hold up this on me.

amateescu’s picture

  1. +++ b/core/modules/node/src/NodeViewsData.php
    @@ -32,7 +31,8 @@ public function getViewsData() {
    +    $data['node_field_data']['title']['field']['default_formatter_settings'] = ['entity_link' => TRUE];
    

    Shouldn't this be 'link_to_entity'?

  2. +++ b/core/modules/views/config/schema/views.field.schema.yml
    @@ -182,3 +182,50 @@ views.field.language:
    +  ¶
    

    Empty space :)

  3. +++ b/core/modules/views/src/EntityViewsData.php
    @@ -308,21 +312,24 @@ protected function mapSingleFieldViewsData($table, $field_name, $field_type, $co
    -        $views_field['field']['id'] = 'url';
    +        // Let's render URIs as URIs by default, not links.
    +        $views_field['field']['id'] = 'field';
    +        $views_field['field']['default_formatter'] = 'string';
    

    I'm wondering what's the reason for this.

  4. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_group_rows.yml
    @@ -4,6 +4,12 @@ dependencies:
       module:
         - field
         - node
    +  config:
    +    - field.storage.node.field_views_testing_group_rows
    +  module:
    +    - field
    +    - node
    +    - user
    

    Why are we adding a new 'module' key instead of appending 'user' to the previous one?

  5. +++ b/core/modules/views/tests/src/Unit/Plugin/field/FieldTest.php
    @@ -136,11 +136,38 @@ public function testDefineOptionsWithNoOptions() {
       }
    +  /**
    +   * @covers ::defineOptions()
    +   */
    +  public function testDefineOptionsWithDefaultFormatterOnFieldDefinition() {
    

    Missing empty line.

dawehner’s picture

FileSize
60.88 KB
2.28 KB

Thank you for the review!

I'm wondering what's the reason for this.

We want to render the raw URL, not a rendered version of it by default.

Why are we adding a new 'module' key instead of appending 'user' to the previous one?

Fixed

plach’s picture

Status: Reviewed & tested by the community » Needs work

Overall 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.

plach’s picture

One 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?

Gábor Hojtsy’s picture

I 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.

alexpott’s picture

  1. +++ b/core/modules/node/config/install/views.view.content.yml
    @@ -222,11 +224,12 @@ display:
    -          type: published-notpublished
    
        $data['node_field_data']['status']['field']['output formats'] = [
          'published-notpublished' => array(t('Published'), t('Not published')),
        ];
    

    And

        $data['node_field_revision']['status']['field']['output formats'] = [
          'published-notpublished' => [t('Published'), t('Not published')],
        ];
    

    in NodeViewsData can be removed.

  2. +++ b/core/modules/user/config/install/views.view.user_admin_people.yml
    @@ -292,11 +292,12 @@ display:
    -          type: active-blocked
    

    Can remove

        $data['users_field_data']['status']['field']['output formats'] = array(
          'active-blocked' => array(t('Active'), t('Blocked')),
        );
    

    from UserViewsData (i think).

  3. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_group_rows.yml
    @@ -4,6 +4,10 @@ dependencies:
    +  module:
    

    Spurious line?

Gábor Hojtsy’s picture

(BTW that is not required to commit this step IMHO).

jhodgdon’s picture

I'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.

jhodgdon’s picture

@alexpott - I do not understand your review in #148, items 1 and 2.

jhodgdon’s picture

FileSize
13.05 KB

So 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.
Preview of field-based view

dawehner’s picture

Status: Needs work » Needs review
FileSize
64.26 KB
3.59 KB

@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.

- 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.

Alright, though this is a separate bug, unrelated to this issue #2428103: String formatter should link to its translation.

- 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.

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

plach’s picture

IS update :(

jhodgdon’s picture

OK... 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?

jhodgdon’s picture

I 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. :(

Status: Needs review » Needs work

The last submitted patch, 153: 2342045-153.patch, failed testing.

jhodgdon’s picture

Actually 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...

jhodgdon’s picture

FileSize
12.1 KB

OK, 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.

dawehner’s picture

I'm going to give this some more testing...

Thank you for doing that!

Note: I forgot to press the commend form, so I haven't read #159 yet.

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?

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.

jhodgdon’s picture

Agreed 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.

dawehner’s picture

Thank 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:

SELECT node.nid AS nid
FROM 
{node} node
WHERE (( (node.type IN  ('article')) ))
LIMIT 10 OFFSET 0

The row language display has the following SQL query:

SELECT node_field_data.langcode AS node_field_data_langcode, node.nid AS nid
FROM 
{node} node
INNER JOIN {node_field_data} node_field_data ON node.nid = node_field_data.nid
WHERE (( (node.type IN  ('article')) ))
LIMIT 10 OFFSET 0

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:

  • Enforce to always have a join to {node_field_data}. This would certainly solve the symptom, but is hacky IMHO.
  • Never query {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.

  • Get rid of the separation of {node} and {node_field_data} ... I'm not aware of all the reasons we have that separation, but
    one 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.
dawehner’s picture

Status: Needs work » Postponed
Related issues: +#2429447: Use data table as views base table, if available.

Alright, opened an issue for that problem, as talked on IRC: #2429447: Use data table as views base table, if available.

xjm’s picture

dawehner’s picture

FileSize
64.26 KB

Just a reroll to not fall too much behind.

Gábor Hojtsy’s picture

Issue summary: View changes
dawehner’s picture

FileSize
64.26 KB

Continous rebasing.

dawehner’s picture

Status: Postponed » Needs review
FileSize
63.95 KB

*thumbs*

jhodgdon’s picture

So... 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...

effulgentsia’s picture

Now that #2429447: Use data table as views base table, if available. landed, is this part of the issue summary still true?

We probably will need new formatters, and won't be able to use the Field UI handlers, because a field handler in Views takes care of both query and display, and the database table/field structure for the base fields is not the same as the Field UI fields.

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.

Status: Needs review » Needs work

The last submitted patch, 168: 2342045-168.patch, failed testing.

dawehner’s picture

We probably will need new formatters, and won't be able to use the Field UI handlers, because a field handler in Views takes care of both query and display, and the database table/field structure for the base fields is not the same as the Field UI fields.

Well, the statement that the views field handlers do more than the field API formatters is still true, see custom date handling as an example.

dawehner’s picture

Status: Needs work » Needs review
FileSize
64.75 KB
5.85 KB

Let's fix also the failures.

Gábor Hojtsy’s picture

I 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.

dawehner’s picture

Issue summary: View changes

Worked on a better and more up to date issue summary, which clarifies more, hopefully.

jhodgdon’s picture

Title: Views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency » Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency
Issue summary: View changes

Tweaking summary a bit and title.

dawehner’s picture

FileSize
64.45 KB
617 bytes

Reviewed some stuff, but could not find many things.

jhodgdon’s picture

So. 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?

dawehner’s picture

FileSize
65.21 KB
773 bytes

Yeah I think we should ensure that this really works.

Here is a small cleanup which we could do.

dawehner’s picture

FileSize
65.21 KB

Rerolled

Status: Needs review » Needs work

The last submitted patch, 180: 2342045-180.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
65.2 KB

Another proper reroll.

dawehner’s picture

Issue summary: View changes
plach’s picture

Skimming through the patch I found a few more (very) minor issues:

  1. +++ b/core/modules/views/src/Plugin/views/query/Sql.php
    @@ -1504,7 +1507,7 @@ function loadEntities(&$results) {
    +      if (isset($entity_information[$entity_type]['revision']) && $entity_information[$entity_type]['revision'] == TRUE) {
    

    "Nitpick: Afaik we use more === these days." (cit.) ;)

  2. +++ b/core/modules/views/src/Tests/Handler/FieldFieldTest.php
    @@ -125,4 +184,53 @@ public function testSimpleRender() {
    +    $this->assertIdenticalResultset($executable, [
    +      ['id' => 1, 'field_test' => 1, 'revision_id' => 1, 'name' => 'base value'],
    +      ['id' => 1, 'field_test' => 2, 'revision_id' => 2, 'name' => 'revision value1'],
    +      ['id' => 1, 'field_test' => 3, 'revision_id' => 3, 'name' => 'revision value2'],
    +      ['id' => 2, 'field_test' => 4, 'revision_id' => 4, 'name' => 'next entity value'],
    +    ],
    +      ['entity_test_rev_revision_id' => 'id', 'revision_id' => 'revision_id', 'name' => 'name', 'field_test' => 'field_test']
    

    Weird indentation: can we indent all arguments, and use two indentation levels for the array items of the second argument?

  3. +++ b/core/modules/views/src/Tests/Handler/FieldGroupRowsWebTest.php
    @@ -0,0 +1,124 @@
    +        'field_name' => $this->fieldName,
    +        'entity_type' => 'node',
    +        'type' => 'text',
    +        'cardinality' => FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED,
    +    ]);
    

    Wrong indentation.

Doing some more testing now.

dawehner’s picture

FileSize
106.3 KB
43.83 KB

Went through all views.view.*.yml files

Status: Needs review » Needs work

The last submitted patch, 186: 2342045-186.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
106.32 KB
508 bytes

Fix at least one schema failure.

dawehner’s picture

Fix at least one schema failure.

Status: Needs review » Needs work

The last submitted patch, 188: 2342045-188.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
109.29 KB
7 KB

More schema fixes.

dawehner’s picture

Did 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...

Wim Leers’s picture

Does 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?

Fabianx’s picture

Factor 2, ouch that is hard ... I think yes, we might need to tackle #1867518: Leverage entityDisplay to provide fast rendering for fields first ...

jhodgdon’s picture

The 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?

Wim Leers’s picture

#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.

dawehner’s picture

FileSize
119.88 KB
13.8 KB

"Nitpick: Afaik we use more === these days." (cit.) ;)

Yeah, let's do that.

Weird indentation: can we indent all arguments, and use two indentation levels for the array items of the second argument?

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

dawehner’s picture

Issue summary: View changes

Adapted the issue summary regarding #196, feel free to improve it.

dawehner’s picture

FileSize
119.88 KB
2.52 KB

You should simply not trust tests without running them.

The last submitted patch, 197: 2342045-194.patch, failed testing.

jhodgdon’s picture

The test looks good to me! Also the issue summary update.

plach’s picture

Issue summary: View changes
dawehner’s picture

FileSize
119.38 KB
1.93 KB

Some small bits.

plach’s picture

Status: Needs review » Needs work

Looks good to me too! Some other nitpicks:

  1. +++ b/core/modules/views/src/Tests/Entity/FieldEntityTranslationTest.php
    @@ -0,0 +1,182 @@
    +  public function testTranslationRows() {
    

    Missing PHP doc

  2. +++ b/core/modules/views/src/Tests/Entity/FieldEntityTranslationTest.php
    @@ -0,0 +1,182 @@
    +    $this->drupalGet('es/test_entity_field_renderers/language_interface');
    

    We should pass new Language('es') as an option to ::drupalGet().

dawehner’s picture

Status: Needs work » Needs review
FileSize
119.65 KB
1.26 KB

Thank you for your review!

plach’s picture

This looks ready to me.

@Wim: I'd like to RTBC this, does the issue summary look compelling now?

plach’s picture

Status: Needs review » Reviewed & tested by the community

@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.

The last submitted patch, 165: 2342045-165.patch, failed testing.

The last submitted patch, 167: 2342045-167.patch, failed testing.

Fabianx’s picture

Assigned: Unassigned » catch
Issue tags: -needs profiling +Performance regression, +Needs change record

We 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!

Wim Leers’s picture

+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!

plach’s picture

Thanks 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.

catch’s picture

Opened #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.

joshtaylor’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
125.1 KB

Straight reroll.

Status: Needs review » Needs work

The last submitted patch, 214: 2342045-214.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
121.06 KB
2.44 KB

Let's fix things for now, but clean up stuff much better in #2451679: Validate cache contexts (+ cache contexts in some views plugins wrong)

plach’s picture

Status: Needs review » Reviewed & tested by the community

  • catch committed 4b812ec on 8.0.x
    Issue #2342045 by dawehner, Gábor Hojtsy, larowlan, joshtaylor: Standard...
catch’s picture

Status: Reviewed & tested by the community » Fixed

OK 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!

dawehner’s picture

Thank you a lot!!

YesCT’s picture

this 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

Gábor Hojtsy’s picture

Updated and published https://www.drupal.org/node/2452797, thanks for the draft!

Thanks all, especially @dawehner for the incredible work here! Amazing!

jbrown’s picture

Thanks everyone for working so hard to fix this issue!

yched’s picture

Impressive work, @dawehner !

Berdir’s picture

Somehow 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 :)

xjm’s picture

Assigned: catch » Unassigned

SO awesome.

Status: Fixed » Closed (fixed)

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

apaderno’s picture

Issue tags: -Performance regression +Performance