🇦🇷Argentina @anairamzap

anairamzap → made their first commit to this issue’s fork.

oh, interesting! I assumed the method in core was "right" and contrib should mimic that signature change.

I'm wondering which is the best approach to avoid this error on panels in sites that did not updated core yet... Maybe apply this patch until core is updated?

anairamzap → created an issue.

ok, so... I've tested https://git.drupalcode.org/project/amp/-/merge_requests/17 in one of our sites dev environment that requires to have a cookie setup in order to load the pages and the guzzle approach is failing with a 403 since that request does not have the cookie set. I imagine a similar thing would happen on sites behind a proxy and we know is also happening on a local docker environment.

I'll try to modify the MR re-writing the render method. Setting back to needs work.

Just a comment to say we are experiencing this (css/js assets not present when using aggregation) when using AMP contrib module.

I have already related the issue 🐛 AMP prevents css assets from generating when aggregation is enabled on Drupal 10.1 Needs review but I wanted to also let everyone here know that we have another failed case with a contrib module, maybe not so popular as symfony_mailer, but still 2.5k users might be affected by this (if they enable aggregation).

Hopefully the idea of adding a new method to be able to generate the aggregated files is reconsidered.

Please note that the patch attached in https://www.drupal.org/project/amp/issues/3374942#comment-15152404 🐛 AMP prevents css assets from generating when aggregation is enabled on Drupal 10.1 Needs review tries to use the guzzle request approach (mentioned in https://www.drupal.org/project/symfony_mailer/issues/3371042 📌 Drupal 10.1.0 new aggregation breaks InlineCssEmailAdjuster Needs review ) , but that's failing in too many scenarios:

• Local docker env
• Sites behind a proxy
• Sites requiring a cookie to grant access

Since we need amp for one of our client's site I'll be working on the related issue trying to add a workaround that ideally doesn't disable the aggregation for amp assets, but if we can not achieve that we will need to simply ignore aggregation in order to get amp pages working (even with a bad performance result).

I've created a MR that uses the provided patch at #2 🐛 AMP prevents css assets from generating when aggregation is enabled on Drupal 10.1 Needs review with the following changes:

1. Replacing Drupal static calls with dependency injection
2. Removing the $needs_rewrite check, so we always send the assets to the doRewrite() method. I've checked and without that, the custom fonts won't be loaded (404) since we were trying to get them using the relative one level up../. Screenshot attached trying to explain this error.
3. Removing the message inside the catch, replacing it with a proper logger if something goes wrong in the curl guzzle request.
4. Also added a to-do comment to the guzzle request since this was failing on my local env (using docker localhost)

I will also provide, in a separate branch another "solution" for this css not loading issue. Being more like a workaround to be able to keep using the file_get_contents approach, simply ignoring aggregation for css (exactly as this module is doing with the javascript) until we can find a working solution that applies to all environments (please not the curl guzzle request will also fail if sites are behind a proxy).

anairamzap → made their first commit to this issue’s fork.

anairamzap → created an issue.

Hello, the last modifications to the MR include a syntax error on a comment :S

In the core/modules/menu_ui/src/MenuForm.php file line 234

diff --git a/core/modules/menu_ui/src/MenuForm.php b/core/modules/menu_ui/src/MenuForm.php
index 9d90ac5a607d90f27b5776b722ed9f4feb60fefb..86fa319c74c4c19ececbf74aea8e84308faedd38 100644
--- a/core/modules/menu_ui/src/MenuForm.php
+++ b/core/modules/menu_ui/src/MenuForm.php
@@ -231,7 +231,13 @@ protected function buildOverviewForm(array &$form, FormStateInterface $form_stat
     // We indicate that a menu administrator is running the menu access check.
     $this->getRequest()->attributes->set('_menu_admin', TRUE);
     $manipulators = [
-      ['callable' => 'menu.default_tree_manipulators:checkAccess'],
+   + // Use a dedicated menu tree access check manipulator as users editing
+   * // this form, granted with 'administer menu' permission, should be able to
+   * // access menu links with inaccessible routes. The default menu tree
+   * // manipulator only allows the access to menu links with accessible routes.
+      // @see \Drupal\Core\Menu\DefaultMenuLinkTreeManipulators::checkAccess()
+      // @see \Drupal\menu_ui\Menu\MenuUiMenuTreeManipulators::checkAccess()
+      ['callable' => 'menu_ui.menu_tree_manipulators:checkAccess'],
       ['callable' => 'menu.default_tree_manipulators:generateIndexAndSort'],
     ];

I'll try to fix that now on the MR

anairamzap → created an issue.

This makes sense, but note that this feature makes no sense for this display. You don't attach google news to anything else, it's a standalone URL, there is no need for users to access it.

@berdir would you recommend to remove that method entirely then?
If we do that I'm wondering what would happen with existing view displays that attach the Feed display to a Page.

Set to Needs Review since a MR is provided.

anairamzap → created an issue.

Provided a MR to fix the mentioned fixes.

anairamzap → created an issue.

I'm correcting the patch provided in #184 📌 Allow AJAX to use GET requests Fixed since when I did the re-roll I forgot to:

• Add the javascript changes to the *es6.js files
• Build js with yarn build:js
• Commit the changes after the build

We need to do those steps since Drupal 9.5.x still uses the build commands :)

Checked the new patch changes running bash core/scripts/dev/commit-code-check.sh in local and all checks passed OK, so hopefully they also pass in drupalci.

Changing to "needs review" since a MR was provided applying most of the requested changes from last review.

Also in here I'm adding the re-rolled patch for 9.5.x that hopefully now passes the tests...

ok, so... I think I've found the issue with the tests failing. The test fails because the view is never loaded.

Both tests, the one for the plugin at core/modules/views/tests/src/Functional/Plugin/ArgumentDefaultTest.php AND the one for node module at core/modules/node/tests/src/Functional/Views/DateArgumentDefaultTest.php are trying to use the same view wih id test_argument_default_date the problem is the view itself, the config for it, is added on the views_test_config test module.

If you look at the DateArgumentDefaultTest setUp method, is using a different test module: node_test_views which does not provide the expected view and then of course it fails to load it.

Now I'm really not sure if we should either:

1. Duplicate the view config, and add it to the node_test_views module.
2. Or try to re-utilize the view from the views_test_config module.

For the second option (which seemed better so we do not duplicate a view config in a different directory) I've tried to load that module in the DateArgumentDefaultTest setUp method, something like:

  protected function setUp($import_test_views = TRUE, $modules = ['views_test_config']): void {
    parent::setUp($import_test_views, $modules);

but I've got:

1) Drupal\Tests\node\Functional\Views\DateArgumentDefaultTest::testArgumentDefaultNodeCreated
Drupal\Core\Entity\EntityStorageException: 'view' entity with ID 'test_argument_default_date' already exists.

/var/www/html/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php:553
/var/www/html/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php:517
/var/www/html/web/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:253
/var/www/html/web/core/lib/Drupal/Core/Entity/EntityBase.php:339
/var/www/html/web/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:608
/var/www/html/web/core/modules/views/src/Tests/ViewTestData.php:52
/var/www/html/web/core/modules/views/tests/src/Functional/ViewTestBase.php:49
/var/www/html/web/core/modules/node/tests/src/Functional/Views/NodeTestBase.php:21
/var/www/html/web/core/modules/node/tests/src/Functional/Views/DateArgumentDefaultTest.php:78
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:703

So now I'm trying the other approach, but it really doesn't seem like a good idea.
If anyone here could provide some input on this, it would be great :)

Thanks,

Mariana

Hello,
We were using the patch on #169 🐛 UI fatal caused by views argument handlers no longer can provide their own default argument handling Needs work and we could not apply it when updating to Drupal 9.5.

So I'm adding a re-roll of #169 to apply cleanly on 9.5.x in case is useful

Hello, we needed this patch on 9.5 so I took the patch in #177 📌 Allow AJAX to use GET requests Fixed and re-rolled it to apply to 9.5.x.

Attaching it here in case is helpful for someone else.

Thanks!

m

anairamzap → created an issue.

anairamzap → created an issue.

Thanks for providing a patch, I'm adding a small tweak on the hook_config_schema_info_alter to add labels to both mappings.

Providing a Label for the $definitions['user.mail']['mapping']['password_reset_rest'] gives better usability on the "Configuration translation" form.

Adding a Label to the schema:

vs without label:

Hey, thanks for adding the computed field to expose the Read Time in JsonAPI :)
I've found a tiny bug in the MR: When setting the computed value, is not clearing the `words` property so the words are adding up!

We've found this issue when using jsonapi/node endpoint to list a bunch of nodes with their node_read_time field.
Example url: {domain}/jsonapi/node/{content_type}?filter[status]=1&fields[node--{content_type}]=node_read_time on a site using Varnish cache, so the wrong (added up minutes) values were being cached by Varnish showing crazy amount of minutes for anonymous users, while logged-in users (ie: non varnish cached) were getting the 'correct' times.

I'm adding a commit with the clear words line that should hopefully fix this.