🇩🇪Germany @dawehner

quietone → credited dawehner → .

I’m happy for anyone taking over any of my previous work. I don’t have the time to maintain them anymore

lauriii → credited dawehner → .

quietone → credited dawehner → .

quietone → credited dawehner → .

Joachim Namyslo → credited dawehner → .

smustgrave → credited dawehner → .

Let's not talk about graphql here. Its totally out of scope and would require a solid year of work probably upfront.

You can easily use a view for this purpose: https://gitlab.com/dawehner/admin_ui_elm/blob/8.x-1.x/config/install/vie... but I think it really doesn't matter how we end up implementing it. Its really just an implementation detail, and it actually more important to figure out which APIs are missing.

OH I guess I wanted to RTBC it.

This looks perfect for me.

1. +++ b/core/modules/node/config/optional/views.view.content.yml
   @@ -60,10 +60,8 @@ display:
   -            edit_node: edit_node
   -            delete_node: delete_node
   -            dropbutton: dropbutton
   -            timestamp: title
   +            langcode: langcode
   +            operations: operations
              info:
                node_bulk_form:
                  align: ''
   @@ -105,28 +103,14 @@ display:
   
   @@ -105,28 +103,14 @@ display:
                  separator: ''
                  empty_column: false
                  responsive: priority-low
   -            edit_node:
   +            langcode:
                  sortable: false
                  default_sort_order: asc
                  align: ''
                  separator: ''
                  empty_column: false
                  responsive: ''
   -            delete_node:
   -              sortable: false
   -              default_sort_order: asc
   -              align: ''
   -              separator: ''
   -              empty_column: false
   -              responsive: ''
   -            dropbutton:
   -              sortable: false
   -              default_sort_order: asc
   -              align: ''
   -              separator: ''
   -              empty_column: false
   -              responsive: ''
   -            timestamp:
   

   These changes are perfectly fine, as we have operations down below since a while.

2. +++ b/core/modules/views/src/Plugin/views/field/FieldLanguage.php
   @@ -0,0 +1,34 @@
   +
   +/**
   + * @file
   + * Contains \Drupal\views\Plugin\views\field\FieldLanguage.
   + */
   

   Nitpick: Can be fixed on commit ...

Just some quick feedback, so that catch doesn't feel alone on this issue.

I really like the problems it fixes, like stale CSS files during the deployment.

1. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -118,7 +118,24 @@ public function getCssAssets(AttachedAssetsInterface $assets, $optimize) {
   +      $css = \Drupal::service('asset.css.collection_optimizer')->optimize($css);
   

   Let's ensure to inject the service

2. +++ b/core/lib/Drupal/Core/Asset/CssCollectionOptimizer.php
   @@ -141,6 +101,21 @@ public function optimize(array $css_assets) {
   +    foreach ($css_assets as $order => $css_asset) {
   ...
   +        $theme_name = $this->themeManager->getActiveTheme()->getName();
   +        $query = UrlHelper::buildQuery(['libraries' => $minimal_set, 'theme' => $theme_name]);
   

   This query seems to be the same across the entire foreach

3. +++ b/core/lib/Drupal/Core/Asset/CssCollectionOptimizer.php
   @@ -155,11 +130,13 @@ public function optimize(array $css_assets) {
      protected function generateHash(array $css_group) {
   -    $css_data = array();
   -    foreach ($css_group['items'] as $css_file) {
   -      $css_data[] = $css_file['data'];
   +    $normalized = $css_group;
   +    foreach ($normalized as $order => $group) {
   +      foreach ($group['items'] as $key => $asset) {
   +        unset($normalized[$order]['items'][$key]['weight']);
   +      }
        }
   -    return hash('sha256', serialize($css_data));
   +    return hash('sha256', serialize($normalized));
   

   Does the same problem exist for the JS one as well?

4. +++ b/core/modules/system/src/Controller/AssetControllerBase.php
   @@ -0,0 +1,287 @@
   +    $base_name = basename($file_name, '.' . $this->fileExtension);
   +
   +    $file_parts = explode('_', $base_name);
   +    // The group delta is the second segment of the filename, if it's not there
   +    // then  the filename is invalid.
   +    if (!isset($file_parts[1]) || !is_numeric($file_parts[1])) {
   +      throw new BadRequestHttpException('Invalid filename');
   +    }
   +    $group_delta = $file_parts[1];
   +
   +    // The hash is the third segment of the filename.
   +    if (!isset($file_parts[2])) {
   +      throw new BadRequestHttpException('Invalid filename');
   +    }
   +    $hash = $file_parts[2];
   +
   +    // If the group being requested does not exist, assume an invalid filename.
   +    if (!isset($groups[$group_delta])) {
   +      throw new BadRequestHttpException('Invalid filename');
   +    }
   +    $group = $groups[$group_delta];
   +
   +    // Only groups that are preprocessed will be requested, so don't try to
   +    // process ones that aren't.
   +    if (!$group['preprocess']) {
   +      throw new BadRequestHttpException('Invalid filename');
   +    }
   

   Some of them (2) seem to not depend on the previous calculations

One part, for example the generic revert controller could be totally brought in already into core.

In general I still kinda like the solution to add those fields always, but just let them do nothing, in case we are not on a multilingual site.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/LanguageFormatter.php
@@ -122,9 +122,12 @@ protected function viewValue(FieldItemInterface $item) {
+    // Languages like L

This is a bit truncated ;)

Quick reroll, as it might be still useful in the future.

Let me adapt the title, to be more fair about this.

Added a subissue for the bulk operations side of the problem space: #2484037: Make Views bulk operations entity translation aware →

Well yeah, the current patch improves some of the usability, but it certainly doesn't fix the critical side of this issue.

There we go, here are some fixes.

It does, see \Drupal\views\Plugin\views\filter\FilterPluginBase::storeGroupInput

Here is the corresponding test coverage.

@amateescu and myself talked about that during break and we thought about the following:

• Let views continue to show all translations, because translations are a first class citizen in Drupal now
• Given that we have to fix the entity operations links and bulk operations, well that was already the case anywhere
• We should provide the exposed filter which allows you to filter by current source and specific language

We'll try to build that in order to move forward.

@cpj
Its amazing, and kinda suprises me, that views already have the store the value per session feature :) I totally forgot about that

@cpj
Do you think we should default this exposed filter to filter by the original language?

@cpj
Do you think we should default this exposed filter to filter by the original language?

Right, let's create a first subissue: #2476563: Entity operations links tied to original entity language, ignore everything else →

Let's make the issue summary a bit better to understand.

Thank you @lauriii for working on the issue!

I hope that working on those failures will still be worth, if we fix the problem in the right way.

.

1. +++ b/core/modules/node/config/optional/views.view.content.yml
   @@ -23,6 +23,12 @@ display:
   +        options:
   +          disable_sql_rewrite: false
   +          distinct: true
   +          replica: false
   +          query_comment: ''
   +          query_tags: {  }
   

   Yeah, let's not use distinct but rather filter the query properly.

2. +++ b/core/modules/node/config/optional/views.view.content.yml
   @@ -487,47 +496,6 @@ display:
   -        langcode:
   -          id: langcode
   -          table: node_field_data
   -          field: langcode
   -          relationship: none
   -          group_type: group
   -          admin_label: ''
   -          operator: in
   -          value: {  }
   -          group: 1
   -          exposed: true
   -          expose:
   -            operator_id: langcode_op
   -            label: Language
   -            description: ''
   -            use_operator: false
   -            operator: langcode_op
   -            identifier: langcode
   -            required: false
   -            remember: false
   -            multiple: false
   -            remember_roles:
   -              authenticated: authenticated
   -              anonymous: '0'
   -              administrator: '0'
   -            reduce: false
   -          is_grouped: false
   -          group_info:
   -            label: ''
   -            description: ''
   -            identifier: ''
   -            optional: true
   -            widget: select
   -            multiple: false
   -            remember: false
   -            default_group: All
   -            default_group_multiple: {  }
   -            group_items: {  }
   -          plugin_id: language
   -          entity_type: node
   -          entity_field: langcode
   

   So we would that move that querying into the new translation view ... @jhodgdon @gabor Are you okay with that?

My thoughts about this are:

• Use the current content language of the side, if available for the row, otherwise fallback to the default langcode
• Don't show an exposed filter or any additional configuration
• At the same time, make a new issue, see #2474013: Make a node translation view → to show translations

Problem of the configuration

One problem which we might have here is that a lot of the functionality we would need is coupled to the language module, which is not enabled by default,
so adding a filter could cause problems. On the other hand, I don't see a reason that it might not be possible to add a language filter inside views itself.

d) Maybe for multilingual we need a second view that shows node translations rather than individual nodes, and allows bulk/row ops appropriate to translations? In this view you could definitely filter by language, and you'd want the view to show each node in the row language. The ops would be something like "edit this translation" and "delete this translation".

Let's do that bit separately, I think that would be okay ...

The usage of the toolbar as primary navigation helps a lot here already, as people either see, or don't see the "People" menu item, depending on their access. The problem
on the admin/overview pages though still exists.

I guess though that this can also be moved to 9.x

The issue summary should really describe at which point drush starts and when the console ends.

@mile23
Once we reach an agreement that we ant to have the symfony console in core, we should open on issue to pull in the code.

Ups.

Mark will come up with a better name anyway if needed.

I really wonder whether it is worth to make an interface here and I can't get some sleep.

@freelock

Would it be possible to fix at least the code-style issues, maybe this will trigger people with knowledge to node access to review it.

Thank you manuel for the feedback, I guess the following line is problematic.

Instead of checking for an empty join type we should probably check for an INNER join type, as then we want to handle it the strict way.

if (empty($tableinfo['join type'])) {

In order to get this into Drupal the process is to first get it into Drupal8 and then backport which is pretty straighforward once there is a good test coverage.

Thank you for working on this issue!

+++ b/core/lib/Drupal/Core/Database/Query/SelectExtender.phpundefined
@@ -55,6 +55,7 @@ public function uniqueIdentifier() {
+    $this->query->nextPlaceholder();

Couldn't we just return $this->query->nextPlaceholder(), just wondering.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessSubqueryTest.phpundefined
@@ -0,0 +1,229 @@
+   * ¶
...
+    ¶
...
+    ¶

Some trailing whitespace is left.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessSubqueryTest.phpundefined
@@ -0,0 +1,229 @@
+    // Test #1349080

We could instead of this set a // @see http://drupal.org/node/1349080

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessSubqueryTest.phpundefined
@@ -0,0 +1,229 @@
+    // Join on $join_table
...
+    // Now add subquery join

A trailing dot should be added here.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessSubqueryTest.phpundefined
@@ -0,0 +1,229 @@
+    // test as admin_user.

Uppercase T please.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessSubqueryTest.phpundefined
@@ -0,0 +1,229 @@
+      "Admin user should get $expected_admin_count rows returned after initial load. Actual: ".count($untagged));
+    $this->verbose('Admin query result: '.
+      theme('table',array('header'=>array('nid','title','subquery_nid','subquery title'),
+        'rows'=>$untagged)
+      )
+    );
+    $this->verbose('Db Query used: '.print_r($result->queryString,1));

If we have something with a dynamic number we can use format_string() for the message.

In general this needs some small changes for codestyle.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessSubqueryTest.phpundefined
@@ -0,0 +1,229 @@
+    $this->assertEqual(count($tagged), $expected_user_count,
+      "Regular user should get $expected_user_count rows returned after initial load. Actual: ".count($tagged));
+    $this->verbose('Regular user query result: ' .
+      theme('table',array('header'=>array('nid','title','subquery_nid','subquery title'),
+        'rows'=>$tagged)
+      )
+    );
+    $this->verbose('Db Result: '.print_r($result->queryString,1));

I guess we can remove the verbose as it's just used for testing?

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessSubqueryTest.phpundefined
@@ -0,0 +1,229 @@
+   * @return null
+   *
+   * Assertions are made from this function, so no return value necessary.

We probably don't have to document that.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessSubqueryTest.phpundefined
@@ -0,0 +1,229 @@
+        $this->fail('User was able to access a node in the returned result, in the parent query.');
...
+        $this->fail('User was able to access a node in the returned result, in the subquery.');

I guess it would be also helpful to include somehow the actual values so it can be tracked down more easy once this breaks.

+++ b/core/modules/node/node.moduleundefined
@@ -3092,8 +3092,22 @@ function node_query_node_access_alter(AlterableInterface $query) {
+      } else {

The codestyle says: Add a new line for an else{}

+++ b/core/modules/node/node.moduleundefined
@@ -3092,8 +3092,22 @@ function node_query_node_access_alter(AlterableInterface $query) {
+        // Increment the placeholder count on the main query until it matches the placeholders
+        // used by the subquery.
+        $cond_count = $subquery->nextPlaceholder();
+        while ($cond_count--) {
+          $query->nextPlaceholder();

This seems to be really hacky, can't we request a new placeholder and use that?