🇪🇸Spain @tunic

#17, I'm not sure, but if it is related to this issue you can fix it uninstalling xray audit and installing it again. Try that and if the error is gone you are done, if not please crate a new issue.

@ Increweb21 → , thank you for your effort and time updating this page. I'm writing here because I'm not sure about the changes about altering a Paragraph form. It seems Paragraphs has its own way to alter its forms. In that case, I think that information should be in the Paragraph help pages (as it is now at  https://www.drupal.org/docs/contributed-modules/formalter-as-plugin/alte... → as you linked in one of your comments). I think we should not overwhelm new user with information that is not applicable to all forms.

Also, the code that alters the paragraph is hard to understand as it access a form structure that is unknown (or at least I don't picture it). 

I would add that example at the Paragraph help pages (may be updating the previously linked page). Because it can be handy to point users to that help page from here, a link or mention to the paragraph help page could be added (something like : "Take into account that some modules may require their own way to alter some forms. For example, see Paragraph module on altering entity forms")

Let's use the function name and a little explanation for each bullet point instead of using an explanation for half the bullets and the function name for the other half.

Thanks for the quick response, now I can close this. If you have more info that points to Node Authlink please reopen the issue.

tunic → created an issue.

I'm wondering if the module should enable caching.

I mean, the module decorates the EntityAccessCheck but that class does not add any caching information. And given that NodeAuthlink always adds caching information on any AccessResult it means any URL that calls the entity check access uses the cache info added by the module.

I think this may be too drastic. So I think the caching info should only be added for URLs handled by the module or even remove it completely. Anybody that has the URL should be able to access, so no need to cache by user.

Ill try add some automated tests to be sure which approach is the good one.

This module has no JavaScript nor it does any modification on the presentation layer so I don't understand how can it break that dropdown, are you 100% sure it was because this module? Could be another related action to the uninstall process like clearing caches?

I've tested with core 10.3.0-beta1 and Node Auth Link 2.0.0. and dropdown menu works fine.

I've found this issue that may be related: 🐛 drupal 10.1.0 olivero theme dropdown menu not working Postponed: needs info

Can you provide more info to reproduce the bug?

Reviewed changes by  rajesh.vishwakarma → and moved to known problemas again. Thanks for completing the missing information!

Deprecating use of var in favor of let and const. This is already the case in Eslint, see https://www.drupal.org/project/gitlab_templates/issues/3432261  →

Commented on slack: https://drupal.slack.com/archives/C02LJCF78E8/p1715591647872509

Typo

Replace German word with an explanation in English.

#114, I think those are related but different headers. In this issue we are handling the headers X-Drupal-Cache-Tags and X-Drupal-Cache-Contexts that are sent by Drupal core when the cacheability debug functionality is on (http.response.debug_cacheability_headers: true). However, the links you provide refer to the Cache-Tags and Purge-Cache-Tags headers (each purger module uses one or the other).

While the information they carry is similar (the cache tag information) and all are affected by the same problem (header too large), the purger headers are not affected by the fix proposed here because that later headers are added by each purger module itself.

It can be interesting to propose a similar fix on the purger modules; however, it will probably be harder for Varnish to process a single header or multiple headers with an incremental sequence (Cache-Tags-1, Cache-Tags-2, etc) so I guess this is something that must be discussed.

Thanks to all the people working in this.

I wanted to use this fix but the patch can't be use directly in composer. For example, if you add the patch from #36 to the patches section in composer.json composer is not able to patch drupal core because it can't find the files to patch. I think this is because the drupal/core project has a special treatment in composer.json (paths and so).

To make it work with a project using composer you have to modify the patch. I'm adding here the patch I'musing as text (based on #36), so it is not tested by the test run avoiding introducing noise to the issue. This is to help any other people that wants this functionality in their project.

Let me be clear about this: the following patch is for people that want to integrate this fix in a Drupal project manged by composer, this patch is not intended to be committed.

diff --git a/assets/scaffold/files/htaccess b/assets/scaffold/files/htaccess
index 116acf42fb..a46e1a871d 100644
--- a/assets/scaffold/files/htaccess
+++ b/assets/scaffold/files/htaccess
@@ -3,7 +3,7 @@
 #

 # Protect files and directories from prying eyes.
-<FilesMatch "\.(engine|inc|install|make|module|profile|po|sh|.*sql|theme|twig|tpl(\.php)?|xtmpl|yml)(~|\.sw[op]|\.bak|\.orig|\.save)?$|^(\.(?!well-known).*|Entries.*|Repository|Root|Tag|Template|composer\.(json|lock)|web\.config|yarn\.lock|package\.json)$|^#.*#$|\.php(~|\.sw[op]|\.bak|\.orig|\.save)$">
+<FilesMatch "\.(engine|inc|install|make|module|profile|po|sh|.*sql|theme|twig|tpl(\.php)?|xtmpl|yml)(~|\.sw[op]|\.bak|\.orig|\.save)?$|^(\.(?!well-known).*|Entries.*|Repository|Root|Tag|Template|composer\.(json|lock)|web\.config|yarn\.lock|package\.json|phpcs\.xml\.dist|phpunit\.xml\.dist)$|^#.*#$|\.php(~|\.sw[op]|\.bak|\.orig|\.save)$">
   <IfModule mod_authz_core.c>
     Require all denied
   </IfModule>
diff --git a/assets/scaffold/files/web.config b/assets/scaffold/files/web.config
index b769e45e36..113ffc0201 100644
--- a/assets/scaffold/files/web.config
+++ b/assets/scaffold/files/web.config
@@ -22,7 +22,7 @@
     <rewrite>
       <rules>
         <rule name="Protect files and directories from prying eyes" stopProcessing="true">
-          <match url="\.(engine|inc|install|module|profile|po|sh|.*sql|theme|twig|tpl(\.php)?|xtmpl|yml|svn-base)$|^(code-style\.pl|Entries.*|Repository|Root|Tag|Template|all-wcprops|entries|format|composer\.(json|lock)|\.htaccess|yarn.lock|package.json)$" />
+          <match url="\.(engine|inc|install|module|profile|po|sh|.*sql|theme|twig|tpl(\.php)?|xtmpl|yml|svn-base)$|^(code-style\.pl|Entries.*|Repository|Root|Tag|Template|all-wcprops|entries|format|composer\.(json|lock)|\.htaccess|yarn\.lock|package\.json|phpcs\.xml\.dist|phpunit\.xml\.dist)$" />
           <action type="CustomResponse" statusCode="403" subStatusCode="0" statusReason="Forbidden" statusDescription="Access is forbidden." />
         </rule>

diff --git a/modules/system/tests/fixtures/HtaccessTest/phpcs.xml.dist b/modules/system/tests/fixtures/HtaccessTest/phpcs.xml.dist
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/modules/system/tests/fixtures/HtaccessTest/phpunit.xml.dist b/modules/system/tests/fixtures/HtaccessTest/phpunit.xml.dist
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/modules/system/tests/src/Functional/System/HtaccessTest.php b/modules/system/tests/src/Functional/System/HtaccessTest.php
index 09046c446f..711ef5d042 100644
--- a/modules/system/tests/src/Functional/System/HtaccessTest.php
+++ b/modules/system/tests/src/Functional/System/HtaccessTest.php
@@ -87,6 +87,10 @@ protected function getProtectedFiles() {
       $file_paths["$path/access_test.$file_ext"] = 200;
     }

+    // Ensure development files cannot be accessed.
+    $file_paths["$path/phpcs.xml.dist"] = 403;
+    $file_paths["$path/phpunit.xml.dist"] = 403;
+
     // Ensure composer.json and composer.lock cannot be accessed.
     $file_paths["$path/composer.json"] = 403;
     $file_paths["$path/composer.lock"] = 403;

Updating solution.

BTW, do we need a change record

Ah, of course, I forgot to ask for access rights.

Issue is addressed, code issues have been solved, test-only patch fails as it should and tests passes with patch. I think this can be moved to RTBC but I'll wait for more reviewers.

Changes loosk good to me. However, it would be great if we can run the tests only without the patch to confirm the test works ok (fails when no fix has been applied, passes when it is applied). I tried to run the "Test-only changes" stage at https://git.drupalcode.org/issue/drupal-2844620/-/jobs/1342592 but I says:

This job does not run automatically and must be started manually, but you do not have access to it.

Workspaces is now a stable module, see  https://www.drupal.org/project/drupal/issues/3088643 📌 Mark Workspaces as a stable core module Fixed

I guess we should move this guide to "Core modules" or should we wait until 10.3 is released?

@AleexGreen, from what I've understood and from what I've commented I think you have summed up the current state of the issue perfectly.

Thanks!

Replaced the URL, tested locally, everything seems to work.

This will be part of the release 1.1.1.

tunic → created an issue.

Ok to start with fields that are not basic fields of entities.

Why do this only for nodes when we can do it for all fieldable entities?

Just for reference, this code returns the nodes of a certain content type that have value (not null) on a given field:

$ct_name = "theContentType";
$field_name = "theField";
$node_storage = \Drupal::entityTypeManager()->getStorage('node');

$nids = $node_storage->getQuery()->condition($field_name, NULL, "IS NOT NULL")->condition("type", $ct_name)->execute();

$nodes = $node_storage->loadMultiple($nids);

tunic → created an issue.

Committed, thanks bot!

I agree with @alexpott, if the form cancels validation and there are no errors it seems submit handlers will be run.

This is because FormBuilder::processForm runs submit handlers if form has no errors and is not rebuilding, see line 595:

// If there are no errors and the form is not rebuilding, submit the form.
if (!$form_state->isRebuilding() && !FormState::hasAnyErrors()) {
  $submit_response = $this->formSubmitter->doSubmitForm($form, $form_state);

FormState::hasAnyErrors uses internally a static variable in FormState, $anyErrors. I'm not sure why a static variable is used (maybe because subforms?), but anyway FormState::setAnyErrors is not called anywhere in the MR so that's why I think submit handlers will run.

I think the problem lies in FormBuilder::processForm. It should only run submit handlers if form is not rebuilding AND validation has completed without errors, not just no errors were found.

Either FormBuilder::processForm checks also FormState::isValidationCanceled or FormState provides a way to check validation was completed without errors in a new method. The first approach is easier, but I think is FormState who should be in charge of deciding if the form was completely validated instead of requiring two calls (logic should be inside FormState, not force users of FormState to remember to perform these two checks). However, because the error check uses this static method and an internal static variable, I'm not sure how to do it because I don't know why a static variable is used in the first place.

Also, the name change proposed is a good idea, it's clearer and describes more accurately the proposed behavior.

Ok, I think I've fixed it.

Improve a little bit the explanation of what should be used for transaction isolation.

Ok, I'm dumb, I didn't see you already changed the body by reverting to your previous change.

Let me fix the mess I have done... 

@kent, I've reverted again because this time only the advice was changed but not the body. I've time so I'm going to incorporate the changes you proposed, give me a few minutes.

@kentr, I'm sorry but I'm reverting your changes because they are not complete. The advise about the use of tx_isolation and transaction_isolation is not changed and now it is confusing (look for "In the following sections the tx_isolation is used to name the configuration of the transaction isolation level."). Please edit that part as well.
 

Nice addition, works patch applies with no issues. The module still works. Not moving to RBTC because I was not able to find a way to trigger an error message. I tried editing a node and adding HTML garbage but no error was triggered.

How can I trigger the error?

I forgot to unassign me.

Added myself as supporter.

Marked "List three supporters" task as done.

Moved to RTBC because concerns on #17 were addressed by #19 (first condition should be in next line and proposed text has been updated accordingly).

Released v2.0.1.

Please, ignore 2.0.0 because is a faulty release, I created it against the 1.x by mistake. Drupal.org forbids to remove releases so I can't remove it.

Changes sent to the MR. I have changed more things that expected initially:

Improvements:

• Use StringTranslationTrait so $this->t() is used instead of t()
• If there were no submissions the previous code returned an error. However, REST APIs usually don't consider an empty set an error because the request is fine, it is just the data returned happens to be empty. Now the code returns just an empty set as result.
• Use HttpException for errors (like other plugins from this module). This simplifies error handling.
• Change output structure renaming webform_submissions to just submissions. The URL already indicates this is a webform and I think response should not leak implementation details to API consumers.
• Add parameters to define a time range ("after" and "before"). The timestamp parameter has been removed (or you could say it has been renamed because the "after" parameter works like the "timestamp" parameter) .
• Removed the check for the existence of the webform_id because this is taken care in upper layers (a 404 page is returned if no webform id is provided)

The functionality provided by the MR is working ok for me. However, I think the timestamp parameter is not enough because you can't define a range.

I would like to replace it with an "after" and "before" params so you can define a time range.

I've added some comments to the MR. In general, the patch looks good to me.

Trying to make tx_isolation and transaction_isolation usage clearer.

@ waluyo.umam → , I guess you mean tx_isolation, because AFAIK tr_isolation is not a valid clause.

transaction_isolation is for  MySQL 5.7.20 or higher, tx_isolation is for MariaDB or MySQL 5.7.19 or less. It is explained here:  https://www.drupal.org/docs/getting-started/system-requirements/setting-... →

The purpose is to teach how to display a confirmation form to an user.The usual way is not to attach it os something like that, you just visit it. The confirmation form has its own URL, so it can be displayed whenever is required just visiting the form's URL.

Ops, I missed that.

I've reviewed the change record and the MR changes and the change record seems up to date to me.

Updated summary with three public functions that are API changes in the current MR.

I also think this would need a change record.

Thanks for quick response, smustgrave.

If you mean fixing the schema I guess not because in rest/config/schema/ you can see the schema for the "format" key that is in the same form as the added "pager", so it seems the right place.

If you mean the updating process to add "pager" to all the views missing it I'm not sure but the failing tests doesn't seem to be related to an update process (although I'm not sure how to test a core update using tests).

But.. I'm not very good with schema, so I could be totally wrong. The only option to check this is to move the update process (the rest_update_11001 function) to views.install, I guess...

Any schema expert in the room?

I need help here.

Tests complain about the schema. Examples:

• https://git.drupalcode.org/issue/drupal-2982729/-/jobs/869401#L4367
• https://git.drupalcode.org/issue/drupal-2982729/-/jobs/869399#L4117

However, I did update the schema, see https://git.drupalcode.org/project/drupal/-/merge_requests/6558/diffs#76...

Does the schema needs to de declared elsewhere? Is the updated schema wrong?

Hi vitch. I've checked the docs and I don't see any reference to placeholder. I agree this should be documented. you can open an issue to core to add the documentation, it is probable the best course of action to solve this.

Feedback on #19:

1) I fully agree removing the warning. I spent hours myself trying to solve that warning without luck.

2) Full fix description sounds reasonable sounds reasonable but I don't know all the nuances of cookie behavior so I can't have a strong opinion.

Thanks @Anybody for reply. I'm working with CRZDEV in the same project. While field_permission would work we think is better to have this option in the module because it seems like a usual case: it makes sense that users that are provided access by this module are not able to revoke access by themselves by modifying the "Role access" field.

Your concerns about other access mechanisms is totally legit. The problem is this fix is final on accessing the role field. What I mean is with this fix users would be able to access the role field or not taking only the permission into account (I mean the permission that the fix adds, edit_role_field entity_access_by_role_field permissions). Thus, it won't possible to add more complex logic: you either have access to the field or you don't have access depending on you having or not the permission.

This can be fixed adding a new configuration in the field, something like "Block access to the role field on editing operations to avoid users revoking access by themselves accidentally". When this check is false (so no blocking access to role field) we should return Neutral, allowing complex logic using field permission or other modules. When the check is true, we would return access granted or forbidden depending on the permission (if bypass permission is on user has access). This would imply changing the configuration schema and creating and upgrade path where:

• Bypass permission is not granted to any role.
• The setting to block access to role field is set to false.

I think those changes would be enough to provide the functionality while preserving current behavior.

Would agree on this approach?

+1

+1

Tests still ok.

I've split the render function adding a new wrapRowsInPager function called when the pager is enabled. This makes the main render function cleaner and shorter. Also, it allows to document the new code in the new function header.

Additionally, values in the pager are passed through intval because some of them were return as strings in the JSON response.

Tests passed.

Moving to NR to trigger testing.

I've created an issue fork and committed patch from #54.

I've tried to fix the tests, one comes from the schema configuration, not sure if I did it right.

Let's see what tests say.

tunic → changed the visibility of the branch 11.x to hidden.

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

jlbellido → credited tunic → .

I'm sorry to hear that.

I suggest to stick to release 8.x-1.0-beta4 until you upgrade to Drupal 9.5, then switch to the 2.0.0 release.

Personally, I won't have time to work on this, I would just drop Drupal 8 support on 2.0.0. but patchs are welcome.

Fixed in the 2.x branch.

I'm merging this because branch 2.x is development branch and should not do any harm (at least until a new release).

@anrkaid, if you see any problem please comment it here, thanks!

Changed MR to merge to 2.x branch where 2.0.0 release is being cooked.

See 📌 Release 2.0.0 Active .

• Created new branch 2.x from 8.x-1.x bnranch
• Added fix of commit 727a40b555bf4b47123b5b0443a3d727bef4ea1d that was not present in 8.x-1.x

tunic → created an issue.

tunic → created an issue.

Thank for the quick response, I'll work on this this week.

tunic → created an issue.

The module is almost ready, only info.yml update is requires, as suggested by Project Update Bot, see 📌 Automated Drupal 10 compatibility fixes Needs review .

Closing this.

tunic → created an issue.

I'm sorry  nofue →  but I'm reverting your edition for two reasons:

1. The text includes sentences using 'I' and includes some doubts (like not sure if 440 is enough). I think this wording is probably making people less confident on the solution.
2. I don't understand the problem. robots.txt files are regular content files and they should have the same permissions as other core/modules files like PHP code, for example. I don't see why robots.txt should have a different set of permissions. The proposed scheme by this page propose that a user should be able to write them and webserver should be only able to read (see  https://www.drupal.org/docs/administering-a-drupal-site/security-in-drup... → ). With the command you propose nobody is able to write, so it can impact next deployments where robots.txt are changed.

That's why I think that text should not be in this page. If you think I'm wrong please give details why.

Tested and working ok, command is available and works without issues.

Issue reported by the bot has been corrected, so moving to Needs Review again.

It was much simpler, just rename $configFactory to just $config.

tunic → changed the visibility of the branch 5.0.x to hidden.

tunic → created an issue.

Instead of merging branch 5.x into this MR I've just rebased the MR, so the MR doesn't include itself changes already committed. This was if anyone (like me) wants to use the MR patch can do it without including a bunch of unrelated changes.

There were conflicts with main branch, I've tried to solve them.

Tests still required.

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

Fixed small typo.

Yes, the commit of the error you mention was introduced in the MR of this issue when a rebase was done, adding all commits committed the dev branch.

So we are good here, and still RTBC-

I tried to clarify current state in the issue body: we need test to move this to RTBC.

The MR should be used, ignore patches.

The patch is working for me in a real project.

#10, I think the problem you are pointing it is a different issue. If I apply that patch to 10.2 without the fix in the MR the error described in this issue are still present.

I would say a new issue has to be created.

I've reverted VVS edition because transaction_isolation is only used on MySQL 5.7.20 or higher. MariaDB and previouse versions of MySQL use tx_isolation. This is explained in the page:
 

In the next sections the tx_isolation is used to name the configuration of the transaction isolation level. However, transaction_isolation was added in MySQL 5.7.20 as an alias for tx_isolation, which is now deprecated and is removed in MySQL 8.0. Hence, if you are using MariaDB or MySQL 5.7.19 or a previous release use tx_isolation but use transaction_isolation if you are using MySQL 5.7.20 or higher.

Crediting also CRZDEV because we discussed the solution with him, thanks!

Merged, thanks!

I've created ✨ Improve Admin interface in layout editor page Postponed to solve this issue in a general way. I'm closing this because the issue (modal dialog broken) is fixed, although we need a way to mix admin and main themes to remove the workaround.

tunic → created an issue.

@ressa, I've reviewed it, thanks for contributing! I fixed a small error, let's continue the conversation there if needed. 

Fixing explanation of the created symlink because the link will be at public_html pointing to web folder, not the other way around.

Typos

Eduardo Morales Alberti → credited tunic → .