Idea from #71 would be great. Using an arbitrary big number doesn't ensure issue is not going to happen.
Other option is adding the check in the Form API layer. During form rendering the number of variables needed can be known, and raise a warning to the watchdog. #71 idea is easier to implement, I guess.
Tests the VLS Demo, it installs ok and seems to work. Good split.
Moving to RBTC.
Some comments on the source code:
File src/Commands/YotpoCommands.php
if (empty($options['external_id'])) {
$this->logger->notice('The external_id param is mandatory');
}
A warning is triggered but the function execution continues, if it is mandatory would make sense to throw an exception or exiting the method? Or if it is really not mandatory then remove the notice.
File src/YotpoClient.php
There several calls to $this->logger->notice that I would say they should be info. For example:
public function getYotpoProducts() {
$products = $this->yotpoClient->getYotpoProducts();
$this->logger->notice('Products yotpo: {products}', ['products' => print_r($products, TRUE)]);
}
This is a normal operation with normal execution, no special conditions. However, notice logs are for "Normal but significant conditions".
Info are just "informational messages" and I think it is a better fir for those logs that inform on actions taken by the module.
This comment is mismatched:
/**
* Logger.
*
* @var \Drupal\Core\Config\ImmutableConfig
*/
protected ImmutableConfig $config;
Additionally, the comments are not explanatory. For example,
/**
* Default options.
*
* @var array
*/
protected array $defaultOptions = [
The comment says the same thing as the variable name, so no additional information is provided. I would be nice to say something like "Default options for the Guzzle's HTTP client"
I'm not sure about addDefaultOptions. The code is:
foreach ($options as $key => $value) {
if (isset($this->defaultOptions[$key]) && is_array($options[$key]) && is_array($this->defaultOptions[$key])) {
$options[$key] = array_merge($this->defaultOptions[$key], $value);
}
}
$options += $this->defaultOptions;
So it seems $this->defaultOptions is merged into $options twice.
Some expressions can be simplified for readability:
$headers = isset($options['headers']) ? array_merge($options['headers'], $additional_headers) : $additional_headers;
to
$headers = ($options['headers'] ?? []) + $additional_headers;
This operator is available since PHP 7 so it should be safe to use.
I would split callYotpoApi in two functions to lower the complexity, putting the code that actually requests the info in another function (the code under if (empty($cached_response) || empty($cached_response->data)) {).
In several places the method callYotpoApi is called. This method can throw an exception but the calls are not wrapped in a try-catrch, is this intended? If this is only used inside drush commands I think drush catches the exception and nicely ends the command, so it would be ok.
I would say
$this->eventDispatcher->dispatch(new MenuLinkTreeManipulatorsAlterEvent($tree, $manipulators, $this), MenuLinkTreeEvents::ALTER_MANIPULATORS);
foreach ($manipulators as $manipulator) {
$callable = $this->callableResolver->getCallableFromDefinition($manipulator['callable']);
should be:
$event = new MenuLinkTreeManipulatorsAlterEvent($tree, $manipulators, $this);
$this->eventDispatcher->dispatch($event, MenuLinkTreeEvents::ALTER_MANIPULATORS);
foreach ($event->getManipulators() as $manipulator) {
$callable = $this->callableResolver->getCallableFromDefinition($manipulator['callable']);
The $manipulators array is not changed during dispatch because arrays are copied when assigned, so a call to setManipulators inside a subscriber changes the array inside the $event but not the original $manipulators array used to create the instance of $event.
I would say throwing an exception.
I've searched in one of my projects and only found a few references to executeSubmitHandlers in core, and none on the modules folder (and there are plenty of modules in this project), so it seems it is a very internal function that is not commonly called.
Maybe some modules are impacted, but I guess they will not many, and throwing an exception sounds like the right way.
Add a way to find Form elements available and explain about render and form elements
Updating link to FormElement, adding reference to the new FormElemenBase because the info was wrong as pointed out by brad.bulger → .
Increweb21 , after a few months without answer I've moved your edition to its own page in the Paragraphs documentation. It needs to be approved by a maintainer, though. See https://www.drupal.org/node/3471662/ →
The content of this page has been moved from the "Conditional Form Fields" Form API page → to Paragraph documentation because it is specific to Paragraphs.
Original content written by Increweb21 → . See m,y comment of 17 June 2024 at 10:16 in the discussion page: https://www.drupal.org/node/3066337/discuss →
Thanks for the info @pbonnefoi. In that case there's no need to update the page as the info is correct and the issue is being handled.
Hiding first patch that is now outdated.
The file has a wrong commeny about being part of a package so I'm updating the issue to include that.
Added path to fix this.
tunic → created an issue.
Added fixes for mos of the errors thanks to automatics fixes provided by phpcbf.
To try yourself run:
phpcbf --standard=Drupal web/modules/development/languagewire_translation_provider/
@ManojSelvan, thanks for your comment, in that case this indeed would need to be updated . Can you propose a fix? Or link to the relevant issue or change record of that change?
Thanks for the detailed information, that is very clear.
It seems the Reporting API module may need a fix also because it seems it sets the report-uri directive as well:
https://git.drupalcode.org/project/reporting/-/blob/2.0.x/src/Plugin/Csp...
public function alterPolicy(Csp $policy): void {
/** @var \Drupal\reporting\Entity\ReportingEndpointInterface $reportingEndpoint */
$reportingEndpoint = $this->reportingEndpointStorage->load($this->configuration['endpoint']);
if ($reportingEndpoint && $reportingEndpoint->status()) {
$policy->setDirective('report-uri', $reportingEndpoint->toUrl('log', ['absolute' => TRUE])->toString());
$policy->setDirective('report-to', $reportingEndpoint->id());
}
}
Created MR that removes the trailing char.
@nofue, that's why I talked about "the webserver" and not the owner, group or others. There are many ways to set up the folders and permissions, but it is commonly accepted that the web serve can't write in code folders and files. This way, if the web server is compromised it can't modify apps source code (what would lead to worse problems).
@O'Briat, that's a common problem (drupal creates files under the web server user and group). There is only one solution, as far as i know: run all deploy and drush commands as the web server user. This is not always posible in shared environments and it usually is harder to setup por newbies or not sysadmin users.
You could also use setgid on the writable folders, and let the folders be owned by certain group. This will make all files and folders created to be part of that given group. However, this doesn't solve all cases, becase files maybe created by the web server or the deploy user (for example, drush commands). Because you can't make the folder be part of more than one group (ACL lists apart) you can't configure a set up where files and folder crated by either the deploy user or the web server are writable by both of them (or I am not aware any way to do that).
I agree the page should give advice on folders that should be writable by the web server. I propose to say there two types of files and folders: code and content. Code should be not writable, and content should be. I wouldn't summarize the content folders (apart from saying some examples like files, tmp and private) because that should be done in another page (BTW, I think there's no help page with all the folders Drupal may require).
Replace user "greg" by user "deploy" because it may be less confusing
In my opinion this page should not talk about tmp, private, translation and other folder locations. This is because this page should only care about the file permissions scheme but not about where the folders should be placed. I would create a new page in this "Security in Drupal" guider about folder recommended places regarding security. this way info about permissions is kept here and info about where folders should be is managed in that other page.
Could be interesting to point out here that there two types of folders and files: code and content. vendor, core or modules are code folders while flles, tmp or private are content folders. Each one has a different permission scheme. This distinction is kind of stated in this page (look for the sentence "then give the www-data user the permission to write files only in that directory") but I think it could be much more explicit (example: https://github.com/Metadrop/drupal-fix-permissions-script#details)-
#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.
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
#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.
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
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.