Account created on 22 December 2006, about 17 years ago
#

Recent comments

πŸ‡©πŸ‡ͺGermany dawehner

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

πŸ‡©πŸ‡ͺGermany dawehner

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

πŸ‡©πŸ‡ͺGermany dawehner

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.

πŸ‡©πŸ‡ͺGermany dawehner

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

πŸ‡©πŸ‡ͺGermany dawehner

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

πŸ‡©πŸ‡ͺGermany dawehner

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

πŸ‡©πŸ‡ͺGermany dawehner

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

πŸ‡©πŸ‡ͺGermany dawehner

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

πŸ‡©πŸ‡ͺGermany dawehner

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

πŸ‡©πŸ‡ͺGermany dawehner

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.

πŸ‡©πŸ‡ͺGermany dawehner

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?

Production build https://api.contrib.social 0.61.6-2-g546bc20