🇫🇮Finland @vermario

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

I checked this again, and the function here: https://git.drupalcode.org/project/access_by_taxonomy/-/blob/1.0.x/src/A... already does what we need. So I think we can close this.

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

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

vermario → created an issue.

vermario → created an issue.

Fixed in beta7

vermario → created an issue.

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

vermario → created an issue.

vermario → created an issue.

vermario → created an issue.

Agree with the previous poster, this will become a problem in Drupal11 for sure. Sounds like it would be great to push this change forward.

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

I tried this in my project and it works nicely. Great that we have a test. LGTM!

(the new version breaks the functionality in drupal 10.2.7 , the solution was to downgrade to alpha 3 in our case, then probably upgrade to alpha4 together with the upgrade to 10.3 that we will do in the near future.)

the same happens with 6.0.6 and drupal 10.2.7

Fixed, also added equivalent functionality when a role is removed.

oh, sorry I missed it :-)

(strangely the problem still happened in my deployment even with that piece of code...)

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

I have seen this same issue after deployment with the latest version of the module.

Looking at the code in the commit above, it seems that there's only an empty function, is that intentional? Or did we need something like

  drupal_flush_all_caches();

in there to actually clear the cache?

We encountered this same issue in the context of the module we are developing https://www.drupal.org/project/access_by_taxonomy →

Our module adds grants that are based on the taxonomy terms added to a node (can be role or specific user). We add these grants also based on translation.

The case where we see a problem in views (Compared to the actual access of the node when trying to visit it directly is this):

1. we add a node with some restrictions, saying that it should be visible for a given user role, set the node as unpublished
2. we add a translation to the node, publish the translation

Now, all users of the role that is specified for access should see the node in views: in this case they should not see the original version, but they should see the translation.

When doing an entity query or a view, the translation is not returned, because the table is checked with fallback = 1

This is an example of the query:

SELECT "node_field_data"."created" AS "node_field_data_created", "node_field_data"."nid" AS "nid", "node_field_data"."langcode" AS "node_field_data_langcode"
FROM
{node_field_data} "node_field_data"
WHERE (("node_field_data"."type" IN ('article')) AND ("node_field_data"."langcode" IN ('it'))) AND (EXISTS (SELECT "na"."nid" AS "nid"
FROM
{node_access} "na"
WHERE ((("gid" IN ('0')) AND ("realm" = 'all')) OR (("gid" IN ('2', '3')) AND ("realm" = 'access_by_taxonomy_role')) OR (("gid" IN ('4')) AND ("realm" = 'access_by_taxonomy_user')) OR (("gid" IN ('4')) AND ("realm" = 'access_by_taxonomy_own_unpublished')) OR (("gid" IN ('4')) AND ("realm" = 'access_by_taxonomy_owner'))) AND ("na"."grant_view" >= '1') AND ("na"."fallback" = '1') AND (([node_field_data].[nid] = [na].[nid]))))
ORDER BY "node_field_data_created" DESC
LIMIT 11 OFFSET 0

The node_access table looks like this:

10,en,1,1,access_by_taxonomy_view_any_article,1,0,0
10,it,0,1,access_by_taxonomy_view_any_article,1,0,0
10,en,1,3,access_by_taxonomy_owner,1,0,0
10,it,0,3,access_by_taxonomy_owner,1,0,0
10,en,1,3,access_by_taxonomy_own_unpublished,1,0,0
10,it,0,3,access_by_taxonomy_role,1,0,0

So there is no row with fallback = 1 that grants access, so the italian translation of the node is not shown in the view, even though the user would have access to it.

Adding the patch has the opposite effect, both the unpublished node and the translation show up in the view, which is also what we don't wat.

I wonder if we have found a corner case of the node_access system...

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

Berdir → credited vermario → .

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

vermario → created an issue.

So I think the problem is that in this module we are saying that the list we return cannot be null:

https://git.drupalcode.org/project/graphql_compose/-/blob/2.2.x/src/Plug...

return [
              'translations' => [
                'type' => Type::nonNull(Type::listOf(Type::nonNull(static::type($this->getPluginId())))),
                'description' => (string) $this->t('Available translations for content.'),
              ],
            ];

But the "data producer" in the graphql module can in fact return null if you don't have access to the translation:

https://git.drupalcode.org/project/graphql/-/blob/8.x-4.x/src/Plugin/Gra...

if ($access) {
          /** @var \Drupal\Core\Access\AccessResultInterface $accessResult */
          $accessResult = $entity->access($accessOperation, $accessUser, TRUE);
          $context->addCacheableDependency($accessResult);
          if (!$accessResult->isAllowed()) {
            return NULL;
          }
        }

So we should allow the list to contain also NULL elements (or we can filter them out competely if they are null?) (not sure :-) )

vermario → created an issue.

Great!

One possible improvement would be to add this to the node interface instead of individual bundles.

would this be something like what happens in graphql_compose_metatags?

hi! I saw the same issue you are seeing, but with translations (another situation in which the tabledrag functionality should not be there).

I read your issue only after I found a possible fix, so I made a patch that actually does the same that you mentioned:

One solution is to update the paragraphs_preprocess_field_multiple_value_form() function to remove the correct columns from the table header and each of the rows. However, that seems fragile, as it would have to be updated again if there's another change to multiple value tables in core at some point in future.

Available in 🐛 Weight fields appear when editing a translation even if the order can't be changed Needs review

Your other solution would work with fields with cardinality 1, but for translations I think we still need the other fix. although, I wonder if this works only for drupal 10.2...

It seems that with drupal 10.2 the indexes of the table (both header and rows) are off by one. So this patch restores the previous functionality.

vermario → created an issue.

Hi!

I had the same issue, and this is a problem of the graphql module, not the grapql_compose one.

Development for the grapqhl module is happening on github, so I made a pull request there: https://github.com/drupal-graphql/graphql/pull/1388

You can add those changes as a patch in your composer.json like this:

            "drupal/graphql" : {
                "drupal-graphql#1323: Add check for translation (from Github)": "https://patch-diff.githubusercontent.com/raw/drupal-graphql/graphql/pull/1388.patch"
            },

For me, this fixes the problem.

hestenet → credited vermario → .

hestenet → credited vermario → .

hestenet → credited vermario → .

Here is a patch that seems to work correctly for us, done following the guide at https://www.drupal.org/node/3158256 →

At this line the .once method was called twice, but it does seem to work correctly with just one with the new method, so I went with that.

Please test! :)

vermario → created an issue.

@edmund.dunn

I noticed that in your patch you have included changes to fix these deprecation warning:

Call to deprecated method doesObjectExist() of class Aws\S3\S3ClientInterface: Use doesObjectExistV2() instead

and
Call to deprecated method doesBucketExist() of class Aws\S3\S3Client: Use doesBucketExistV2() instead

I think there are two problems with this:

1. the new method `doesObjectExistV2` has a new parameter that we should include, so just changing the name of the function won't work.
2. The parameter has to do with "delete markers" https://docs.aws.amazon.com/AmazonS3/latest/userguide/DeleteMarker.html . Supporting this could be a good improvement for the module, but we should make a separate issue for it, so we don't introduce more changes in this one, which is about having the module support drupal 10.

For these reasons, I am including a patch that leaves the deprecated methods in place, but adds your other changes.

hi! I am using Drupal core 9.5.5

The issue you are linking to is indeed fixed, and if I use a content type that has for example the body field, that is on the page when the page loads, the issue does not happen.

This ticket is however about the cases there a field with ckeditor5 is not initially there when the page loads, because it is in a paragraph form field which is initially collapsed. When the paragraph "subform" is opened, then ckeditor appears, and my suspicion is that the fix added as part of #3259380 is unable to fix the issue in this different context.

vermario → created an issue.

Fantastic! thank you for fixing this so quickly! 👍

For me it's a bit of a regression to not color the whole toolbar, since the main point of this module is to clearly differentiate between environments to prevent users changing things in the wrong environment (like deleting a section in production when they actually think they are in staging etc).

So I would also suggest to restore the styling of the full toolbar, unless there's some big technical impediment.

Hi! The patch in #2 works but it might create a fatal error in the cases where the file is not present.

This for us happens for example when working locally with a database exported from production, but without having the files.

This updated patch adds a try/catch to prevent the fatal error.

The idea is taken from another patch in the related issue https://www.drupal.org/project/imagick/issues/3018081#comment-14021982 🐛 Using a remote file scheme does not work Needs review

Here's a revised patch against the latest version of the module

Here's a version of the patch that keeps the update function in place.

Should we go with this?