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

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.

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?