Closed (fixed)
Project:
Drupal.org security advisory coverage applications
Component:
module
Priority:
Normal
Category:
Task
Assigned:
Issue tags:
Reporter:
Created:
8 Feb 2024 at 15:28 UTC
Updated:
9 Apr 2025 at 17:19 UTC
Jump to comment: Most recent
Comments
Comment #2
vishal.kadamThank you for applying!
Please read Review process for security advisory coverage: What to expect for more details and Security advisory coverage application checklist to understand what reviewers look for. Tips for ensuring a smooth review gives some hints for a smoother review.
The important notes are the following.
phpcs --standard=Drupal,DrupalPracticeon the project, which alone fixes most of what reviewers would report.Keep in mind that once the project is opted into security advisory coverage, only Security Team members may change coverage.
To the reviewers
Please read How to review security advisory coverage applications, Application workflow, What to cover in an application review, and Tools to use for reviews.
The important notes are the following.
For new reviewers, I would also suggest to first read In which way the issue queue for coverage applications is different from other project queues.
Comment #3
vishal.kadamComment #4
vishal.kadamRelease branch names always end with the literal .x as described in Release branches. The only exception is for the main branch, which is actually not fully supported on drupal.org and should be avoided.
Comment #5
dragels commentedUpdate branch name in this issue and in code repository.
Comment #6
dragels commentedComment #7
vishal.kadam1. Move all files outside folder and delete "webt" folder
2. Fix phpcs issues.
3. FILE: webt/webt.info.yml
core_version_requirement: ^8 || ^9 || ^10The Drupal Core versions before 8.7.7 do not recognize the core_version_requirement: key.
Comment #8
dragels commented@vishal.kadam Thanks for the quick review, I have made required changes in WEB-T project's Drupalcode repository.
Comment #9
dragels commentedComment #10
dragels commentedChanging the issue priority as per issue priorities.
Comment #11
avpadernosrc/Controller/EtranslationCallbackController.php
$this->logger->info("Received late translation ($request_id)");The first argument must be a literal string, not a concatenation of strings. If there is part of the string that is dynamic, the literal string needs to use placeholders.
src/Controller/AboutTabController.php
The correct placeholder for URLs starts with a colon, such as
:linkwhich must be surrounded by double quote characters.Drupal has other way to output complex markup.
#markupis for simple markup, not markup with images and links.Also, what shown in the user interface must be translatable.
The first line in the documentation comment is sufficient. If the documentation comment describes parameters and return value, using
{@inheritdoc}does not have any pro.src/Controller/AboutTabController.php
For translatable markup, there is the
TranslatableMarkup, but there is no need to explicitly create an instance of that class, since it is already returned from$this->t().src/Form/TranslationProviderForm.php
If the form is used to save values in a configuration object, its parent class needs to be
ConfigFormBase.Comment #12
dragels commented@apaderno Thanks for the review, I have fixed these types of issues in my latest commits.
Comment #13
dragels commentedComment #14
avpadernowebt.module
Hook implementations use a different documentation comment.
Entity hooks like this one needs to use the entity passed as parameter, not an entity stored somewhere. If an entity must the flagged for translation, a dynamic property needs to be set for that entity. (The
ContentEntityBaseclass implements the__get()and__set()magic methods.Instead of verifying a method exists, the code needs to verify the object implements
Drupal\Core\TypedData\TranslatableInterface.src/Controller/AboutTabController.php
The
Drupal\Core\Render\Markupclass is internal and must not be used by contributed modules. In this case, it is not even necessary to use that class, since$this->t()already returns an object that implements\Drupal\Component\Render\MarkupInterface.There is not need to add request_stack as dependency and use that property, since:
(Routing API / Route controllers for simple routes)
There is no need to use that property, since the parent class has
cache().There is no need to use that property, since the parent class has
languageManager().Similarly, there is no need to define those properties, for the same reason. Check which methods must be called to initialize the parent properties.
src/Controller/PretranslationProgressController.php
That trait is already used from the parent class, together other traits.
None of those traits needs to be used from
ControllerBasechild classes.The parent class has methods to use the messenger. Check also which methods must be called to initialize the parent properties.
The correct code to add a link in a translatable string is similar to the following code, which uses the url_generator service (which exists also in Drupal 11).
src/Form/PretranslationForm.php
The class defines properties that are not necessary because the parent class already define the necessary methods; it also include methods that child classes need to call to initialize the parent properties.
For inherited methods, the documentation comments are different from the documentation comments this class uses.
Submission form handlers do not need to remove messages from the messenger.
Comment #15
dragels commentedThanks for the review, I have updated the code. Regarding the idea of using __set() method to mark entity for translation - this did not seem to work. I tried:
1. Using __set() without $entity->save() - dynamic field was not preserved in hook_entity_load method
2. Using __set() with $entity->save() - I get 'Update existing 'node' entity revision while changing the revision ID is not supported.' error
3. Calling __set() from hook_entity_presave instead of hook_entity_update or hook_entity_insert, so that entity is saved afterwards.
However, I refactored the method, so that it uses the entity list provided as parameter.
Comment #16
dragels commentedComment #17
dragels commentedComment #18
avpadernoThank you for your contribution and for your patience with the review process!
I updated your account so you can now opt into security advisory coverage for any project you created and every project you will create.
These are some recommended readings to help you with maintainership:
You can find more contributors chatting on Slack or IRC in #drupal-contribute. So, come hang out and stay involved!
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.
I thank the dedicated reviewers as well.
Comment #19
avpadernoComment #21
cmlaraPer SA-CONTRIB-2025-030 versions older than 1.1.0 posses an Access Bypass and DoS. These versions would have been part of the review process.
Tagging as “Vulnerable when approved” for tracking purposes.