Hey @ccrosaz,
This is great, thanks! It works as expected for labels.
I updated the behaviour to cover descriptions as well.
Regard the phpstan error, you might want to configure your own phpstan.neon with rules that fit your project's needs.
I'd research about this topic, study examples and set up own file.
Don't forget to change the status to Needs Review when ready for review.
Hi @dhruvr,
Could you specify the issue that you are facing with phpstan? I'm not sure I understand your question.
Idk what happened, but the MR had more than 1900 commits and 1000+ changes 🙈
For now I fixed the branch keeping the original 5 pending commits as they were.
bbu23 → changed the visibility of the branch 3302453-canonical-metatag-on to hidden.
Let me know if you'd like me to work on this, and if you have any specific requirements or not.
The en-GB and fr-FR are currently hardcoded in the locales javascript. My proposed solution is to simply use the short language code (en, fr etc.) instead of the specific version of the language code, which works well.
We could evolve this in a separate ticket to allow for specific language code as well if needed, but the current implementation should be fixed by simply providing the current language, no need to add en-GB or fr-FR in the javascript file.
Though I must say that the latest version (1.0.0-beta5) works well with ES and EU languages in its current form, probably because of all the t()'s.
Hello @hemangi.gokhale,
Here's a quick feedback:
- Update PHP Annotations to PHP Attributes where applicable (including tests)
- The module is missing the composer.json file. See Add a
composer.json →
file for more info.
- The schema defines the field "to", but it's missing in the default module configuration object
- New modules, which are compatible with Drupal 10 and higher versions are expected to include type declarations in property definitions, and use property promotion
Example:
public function __construct(
protected EntityTypeManagerInterface $entityTypeManager,
protected EntityRepositoryInterface $entityRepository,
protected DateFormatterInterface $dateFormatter,
TranslationInterface $translation,
) {
$this->setStringTranslation($translation);
}
- For a new module that aims to be compatible with Drupal 10 and Drupal 11, I would rather implement hooks as class methods as described in Support for object oriented hook implementations using autowired services → .
The priority cannot be changed back to Normal yet. See Issue priorities → .
I've added the missing schema for swiper_formatter_paragraphs to address my immediate need. However, there's still additional work required for dialog and potentially other cases that need schema definitions.
The MR patch works for me in Drupal 11.2+, though to me this doesn't work without re-saving because in the case of "file" the stored value is integer 0, and it still causes the following error:
variable type is integer but applied schema class is Drupal\Core\TypedData\Plugin\DataType\StringData
After re-saving, the 0 values get converted to string '0'
So, we might need an update hook.
Hi @dhruvr,
- When removing ControllerBase, is it acceptable to replace the messenger service with a dependency-injected service?
Yes. Just make sure to use property promotion.
- My current approach is to keep it file-only, as we can also select this as a formatter for media.
When you say "select this as a formatter for media" I assume you mean "select this as a formatter for the referenced file of a media entity", which I agree with.
To think about it, I was wrong about the dead code, since the parent entity of the file could indeed be a media, but I stand by the other issue which is the hardcoded media type.
Hi,
Below you have my feedback:
- The generated tokens don't expire. It would be recommended to have expiration date time.
- There's incorrect handling of file access:
requirements: _permission: 'access content' _custom_access: '\Drupal\file_access_via_webform\Controller\FileDownloadController::access'
-- Problem 1: The 'access content' is incorrectly used here. The route doesn't deal with nodes, and if used in a member only site this module would fail to allow file downloads to anonymous users even if the intention was to allow it.
-- Problem 2: There is no proper file access validation for the current user, whether it's a permission or a custom access.
-- Problem 3: The "access content" permission is checked again in the custom access function of the Controller.
- In the Controller, there's no point of using the entity type manager to load the file. This step can be skipped by using the ParamConverter in the route definition.
- New modules, which are compatible with Drupal 10 and higher versions are expected to include type declarations in property definitions, and use property promotion. Applies to all classes.
Example:
public function __construct(
protected EntityTypeManagerInterface $entityTypeManager,
protected EntityRepositoryInterface $entityRepository,
protected DateFormatterInterface $dateFormatter,
TranslationInterface $translation,
) {
$this->setStringTranslation($translation);
}
- File: src/Controller/FileDownloadController.php: the public static function create is missing arguments and it returns new self instead of new static
- The following point has already been debated, but I'd like to add a +1 to the "controller class unnecessarily extending ControllerBase". And especially after making it clear that not even entity type manager is needed, there's no point in extending the parent just for the messenger service. The class can just implement ContainerInjectionInterface.
- For a new module that aims to be compatible with Drupal 10 and Drupal 11, I would rather implement plugins as PHP Attributes instead of PHP Annotations as described in Attribute-based plugins → . (plugin classes)
- In the formatter, there's use of dead code:
// If this is a media reference, get the file entity.
if ($item->entity && $item->entity->bundle() === 'document') {
$file = $item->entity->get('field_media_file')->entity ?? NULL;
}
-- Problem 1: This code cannot be executed since the formatter is defined to work with file entities only.
-- Problem 2: If the code is updated to make it work for media entities as well, the use of hardcoded "document" type is not recommended since this type is not mandatory to exist and you can also have other names and IDs for media types that represent files.
- It's unclear to me if and when the debug method is called in src/Plugin/WebformHandler/WebformDownloadRedirectHandler.php or if it's just a dead code. Could you please clarify?
Let me know if you have any questions, and I would appreciate a personal response rather than one generated with AI, as I am taking my time to manually review and write down this feedback.
hehe thanks @vasike, will do!
That would be great, I hope your predictions will become reality :D
Let's change a bit the title, shall we?
I'm starting to have some ideas, though maybe in the end it could be just a simple command similar to what you suggested. I'll process all the information that I'm gathering but I might need to push myself to finish the old pending Drupal 7 source task because I added there something that I'm thinking I could use.
I'm setting the status to Postponed until the related issue is merged.
Thank you for your contribution.
This will be included in version 4.1.0.
Pending change record & documentation updates.
Cool, sounds good.
I'll take it from here to solve the pipeline. Thank you!
Thanks @vasike, that looks correct though I don't know if you wanted to have also the source_menu or not. If you think the target menu is enough for now, that's good enough for me. But I do wonder if we should rename the property to targetMenu or something similar... 🤔
Regarding the pipeline extra failures, don't worry I'll take care of them.
Sure, I understand that.
Yes, let's go for the Event, you can also add the source menu nullable if needed. And I think this should do it for this ticket*.
As for the rest of the discussion, I see the following:
Option to override the destination menu
At the moment, I don't see an immediate need for overriding the target menu. But I remain open to hear specific use cases that people might have in a dedicated ticket. Personally, I'd want to avoid it as much as possible, but who knows? As you said, some cases might arise.
Option to skip importing specific menu items
I do see scenarios where we'd want to skip some menu items, so I find this interesting. In fact, there were situations in the past where I would've liked to skip the deletion of specific items prior to import, but that would be a bit complicated to tackle in this ticket.
*The way I see it is: either we include this in the current ticket, and create a flag (using a method) in the Event to specify if the item should be skipped/process, and this would solve it for the import. Or if we want to develop something "bigger" then it could be done in a separate task.
Hi @vasike,
Thanks for reporting this task.
Since we already have the target menu name available in the function where the MenuImportEvent is available, we can simply add it there to the event, and update the event class to expect this new argument.
While indeed the option to add it the item is also a possibility, I might prefer the object approach. As for the "source menu" we'll have to evaluate the options that we have or just rely on the exported information in the item. The menu_name value is supposed to be the exported menu (which is considered the source menu when imported) at that time. It's overridden a few lines later.
Though we should keep in mind that "source menu" is not a type of information that is mandatory and it's not something that applies globally to all plugins.
bbu23 → changed the visibility of the branch 3493053-file-extention-issue to hidden.
Hi, @vasike,
Thank you for proposing this feature. I will be analysing it since we have the plugins system for destinations and I'm wondering if we could instead create something like "AnotherMenu" Destination. Of course, if this complicates things then either a new command or the update of quick commands should be the way to go, since the functionality is already there and it just needs to be connected.
The plugin system would be nice because it would make it work in both UI and non-UI versions (in theory). Though the quick command would require an update to allow to choose between the two destination plugins.
$import_menu_file = str_replace($export_menu_name, $import_menu_name, $export_menu_file);
I think the menu name replace should not be needed which raw like that could also replace other unwanted things. In the MenuMigrationService I'm always overriding the menu partly because of this usage.
// Always override the menu name, as this can now be controlled in the
// config entities.
$item['menu_name'] = $menuName;
https://git.drupalcode.org/project/menu_migration/-/blob/4.1.x/src/Servi...
Hi @calebtr,
Thank you for investigating & reporting the issues.
I agree to separate issues #2 and #2.
Regarding point 3, I completely agree that it's wrong to use getEditable. We can update the code to prepare both immutable and mutable objects and use one for get and one for set.
E.g.
$this->immutableConfig = $config_factory->get('CONFIG_NAME');
$this->editableConfig = $config_factory->getEditable('CONFIG_NAME');
There's also an issue 4 related to the key module that I didn't have time to tackle: if using the module without key and then you install key for something else, then the opensolr module is affected. I was thinking to have the user manually choose if they want to use key or not, instead of automatic switch which can cause problems both when enabled and disabled.
Great!
P.S. I would appreciate the contribution record update.
Hi, I'm late to the party but this isn't related to Views only.
This issue appears on Node pages as well.
The problem is that on frontpage the links are using the full path instead of the "/" path.
If you place the core language switcher and the dropdown_language switcher on the same page (in frontpage), you will notice that the core switcher is printing the path "/" but the dropdown_language links are printed with whatever the path of frontpage is (e.g. "/node").
For a site the uses both blocks - one for desktop and one for mobile - this behaviour causes a "Links with different destinations" accessibility warning since the links for the same purpose are different.
Thanks, patch provided in comment #4 works for me in Drupal 11.2.3 + Gin Toolbar 3.0.2
Tested patch from comment #66 🐛 Use new cache tag ENTITY_TYPE_list:BUNDLE in Views to improve cache hit rate Needs work in a basic block view that lists published content filtered by a specific Content Type (Drupal 11.2+).
While the code correctly adds the cache tag by bundle, it also keeps the node_list tag which makes the one by bundle redundant. We need to replace the node_list tag completely by the bundle list tag, not add it.
On the other hand, it would be nice to have it work with contextual filters as well.
Idk about automatic MRs, I've never experienced something like that in here. I always create my MRs manually.
You should be able to create the MR here: https://git.drupalcode.org/issue/swiper_formatter-3554570/-/compare/2.1.... by clicking on the "Create merge request" button.
We might need to update the default config to contain the keyboard addition.
Thanks for the detailed information. Could you please open an MR on this ticket even if it conflicts with other existing MRs? In order for us & others to review, there has to be an MR. Thank you!
This can be solved by overriding the template, but I'm creating a merge request to fix the module's template as well.
As per documentation, it is not recommended to use the id attribute, but instead to use the identifier:
Adds a unique identifier for the date picker input. Use this instead of html id attribute.
I'm also removing the label, since my exposed filter already has a label. If this affects other places, then it needs to be addressed, but usually labels should be separate.
The MR 32 was targeting 1.0.x, and was also missing the 'extensions' key and the other plugins, reason why I created a new MR.
bbu23 → changed the visibility of the branch 3493053-the-filevalidateextensions-plugin to hidden.
bbu23 → changed the visibility of the branch 3493053-file-extention-issue to active.
bbu23 → changed the visibility of the branch 3493053-file-extention-issue to hidden.
Hi @jstoller,
Thanks for reporting the issue and providing a fix.
It looks good to me, I'll test when I get the chance.
Hi! Here's my feedback:
- There is no dev release for your project. While not mandatory, it's a highly encouraged practice in the Drupal community. You should consider creating one.
- There is no schema defined for the module's configuration object. See Configuration schema/metadata → for more info.
- I think you should review the information in the composer.json file. A drupal module does not have to require drupal/core. Drupal core 10 requires minimum PHP 8.1, your PHP requirement is redundant and could be removed unless it's above the minimum required by core.
- The info file sets the dependency on 3 modules, but in composer.json it requires multiple. Are they all needed?
- I also believe that the autoload section is not needed.
- In the AjaxCartUpdateSettingsForm.php you are overriding the create method which in higher versions of Drupal 10 it removes the typed Config Manager parameter. Any reasons for that?
- You should consider using the Drupal's gitlab template if possible
Just a tiny few PHPCS left.
FILE: /var/www/html/web/modules/custom/ajax_cart_update/ajax_cart_update.libraries.yml -------------------------------------------------------------------------------------- FOUND 1 ERROR AFFECTING 1 LINE -------------------------------------------------------------------------------------- 22 | ERROR | [x] Expected 1 newline at end of file; 0 found -------------------------------------------------------------------------------------- PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY -------------------------------------------------------------------------------------- FILE: /var/www/html/web/modules/custom/ajax_cart_update/ajax_cart_update.routing.yml ------------------------------------------------------------------------------------ FOUND 1 ERROR AFFECTING 1 LINE ------------------------------------------------------------------------------------ 28 | ERROR | [x] Expected 1 newline at end of file; 0 found ------------------------------------------------------------------------------------ PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY ------------------------------------------------------------------------------------ FILE: /var/www/html/web/modules/custom/ajax_cart_update/ajax_cart_update.info.yml --------------------------------------------------------------------------------- FOUND 1 ERROR AFFECTING 1 LINE --------------------------------------------------------------------------------- 9 | ERROR | [x] Expected 1 newline at end of file; 0 found --------------------------------------------------------------------------------- PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY ---------------------------------------------------------------------------------
Hi @anairamzap,
Thanks for reporting and providing a solution to this issue.
Regarding the 2.1.x branch, in general drupal.org issues are always opened against the latest dev branch. Then it's up to the maintainers to decide if they backport the fix to lower versions or not, depending on the case.
That's a great catch, but since we are on it, your fix would only apply to the Node and Block entities, and at this point we already know that this issue can reoccur for other entities like taxonomies, custom content entities etc.
Without breaking backwards compatibility, could you improve your solution by making it work for content entities? For example blocks, nodes, terms etc they all have one thing in common and that is that their entity class extend EditorialContentEntityBase. From there you can identify an appropriate interface to check against.
Hi! Here's my feedback:
- There is no dev release for your project. While not mandatory, it's a highly encouraged practice in the Drupal community. You should consider creating one.
- The module is missing its composer.json file. See Add a composer.json → file for more info.
- There is no schema defined for the module's configuration object. See Configuration schema/metadata → for more info.
Hi! Quick note: you don't need to tag a new release after every feedback. We're reviewing directly the branch specified in the title of the ticket, in this case 1.0.x.
It is better not to create new releases during these applications, since a review could ask for a change that is not backward compatible with the existing releases. Just using a development version avoids those BC issues.
And, there is no dev release for your project. While not mandatory, it's a highly encouraged practice in the Drupal community. You should consider creating one.
And since your module already has a logo, I suggest moving the image to the repository to make it compatible with
Project Browser →
and having the icon show next to the module title. In the place of the image you could upload screenshots of the functionality or leave it empty.
See
Logo in Project Browser →
.
Hi! Below you have my feedback:
- There is no dev release. While not mandatory, it's a highly encouraged practice in the Drupal community.
- text_clarity_checker.info.yml:
The translations folder and locale keys should be removed since they are intended for custom modules as described in locale.api.php. Consider contributing the translations for your module to drupal.org. See
Contribute to translations →
.
'interface translation project': text_clarity_checker 'interface translation server pattern': modules/custom/%project/translations/%project.%language.po
- The namespace of the Object Oriented hook classes is wrong: it should be Drupal\text_clarity_checker\Hook instead of Drupal\text_clarity_checker\Hooks. See
changelog →
. The hooks are still called because of the .module procedural hooks that are kept for backwards compatibility. You can disable procedural hooks for Drupal 11+ optionally by setting the following in your services YAML file:
parameters: # For Drupal 11.1 and lower. MODULENAME.hooks_converted: true # For Drupal 11.2 and higher. MODULENAME.skip_procedural_hook_scan: true
- src/Plugin/Block/TextClarityCheckerBlock.php: Is there a reason why your block is uncacheable?
/**
* {@inheritdoc}
*/
public function getCacheMaxAge(): int {
return 0;
}
@cristianodemari: Let me ask you a question. What else is needed to obtain security advisory approval?
Everything is described in comment #2 📌 [1.2.1] Taxonomy Overview Needs review and its linked pages.
Sorry, I see no new commits in the default branch.
And I suggest deleting the tag 2.0.0 if it was not released yet, and create it at the right time from branch 2.0.x as the first stable release from version 2.
Bring all new commits since last public tag to the default branch.
Also, you should delete all the other unused branches that have their name look like a tag.
You're welcome, @cristianodemari!
The branch name looks great!
Below you have my feedback from a partial review. I won't manage to get a full one now:
- There is no dev release. While not mandatory, it's a highly encouraged practice in the Drupal community.
- composer.json should be improved. See Add a
composer.json →
file for more info.
- Also, your module seems to depend on a contrib module, which is not required in composer. Either make the module optional, or ensure it's required in composer.
- taxonomy_overview.info.yml: The feedback from comment
#3
📌
[1.2.1] Taxonomy Overview
Needs review
was resolved, but one of the modules now has the incorrect format. Dependencies are prefixed with their project name. The paragraphs' project name is not drupal.
- taxonomy_overview.routing.yml: The permission access content is too open for some of these routes. I don't think this is as intended. This permission is often given to anonymous users as well.
- There are untranslated strings here and there. Examples:
-- src/Controller/TagsOverviewTermGroupController.php:
line 99:
$merge_link = Link::fromTextAndUrl('Merge', $merge_url)->toString();
line 111:
return [
'#type' => 'table',
'#header' => [
'Grouped Similar Terms',
'Operations',
],
'#rows' => $rows,
'#title' => 'Grouped Similar Taxonomy Terms',
];
- src/Form/TagsOverviewTermMergeForm.php: Related to untranslated strings, but also potential wrong usage of $context['results'] + unsanitized user input passed to it. See Creating a batch in form.inc for more info.
$context['results'][] = "Deleted term \"$label\" (tid: $tid)";
...
hi @cristianodemari,
No, I am talking about these branch names:
Take a look at Search API solr module how they transitioned 8.x-3.x to 4.x. Another example is Admin Toolbar. Or let's take Drupal Core which uses both types.
Now it's up to you from the two types of branch naming which one is suited for you, and hopefully follow the Semantic versioning for release tags, but you can either have something like:
2.x - which should cover both minor and patch releases.
or dedicated branch per minor releases:
2.0.x
2.1.x
etc.
And once you do have a final default branch, don't forget to update Gitlab. Currently your default branch is 1.0.0.
I think the Release branches → page could be a bit misleading.
Release branches for modern Drupal projects follow the format {major}.x or {major}.{minor}.x. Drupal 8 and earlier used {API compatibility}-{major}.x.
I encourage you to use a modern format instead of the old 8.x- branch naming.
Hi! I didn't manage to do a full review, but below you have a brief one. This should give an idea:
- There is no dev release. While not mandatory, it's a highly encouraged practice in the Drupal community.
- I can still see some PHPCS warnings when using the "DrupalPractice" standard:
FILE: /var/www/html/web/modules/custom/paragraph_group/paragraph_group.install ------------------------------------------------------------------------------ FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES ------------------------------------------------------------------------------ 9 | WARNING | There must be no blank line following an inline comment 99 | WARNING | Unused variable $version. ------------------------------------------------------------------------------ FILE: /var/www/html/web/modules/custom/paragraph_group/src/Form/ParagroupConfigForm.php --------------------------------------------------------------------------------------- FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES --------------------------------------------------------------------------------------- 89 | WARNING | Unused variable $key. 573 | WARNING | Unused variable $section. 717 | WARNING | Unused variable $section. 723 | WARNING | Unused variable $key. --------------------------------------------------------------------------------------- FILE: /var/www/html/web/modules/custom/paragraph_group/src/Paragroup/ParagroupFieldGroupManager.php --------------------------------------------------------------------------------------------------- FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 2 LINES --------------------------------------------------------------------------------------------------- 211 | WARNING | Unused variable $entity_type. 295 | WARNING | Unused variable $entity_type. 295 | WARNING | Unused variable $bundle. --------------------------------------------------------------------------------------------------- FILE: /var/www/html/web/modules/custom/paragraph_group/src/Paragroup/ParagroupFormData.php ------------------------------------------------------------------------------------------ FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE ------------------------------------------------------------------------------------------ 217 | WARNING | Unused variable $field_name. ------------------------------------------------------------------------------------------
- Kindly review the README file format, including first line, links, sections, headings etc. More in README.md template →
- Object oriented hooks are not supported in Drupal 10 unless they're called from the module file. Your module supports both Drupal 10 and 11, therefore you need to add backwards compatibility as described in the article linked in comment #7 📌 [3.0.x] Paragraph Group Needs review . In its current form, the module hooks are not invoked at all in Drupal 10.
- Also, you don't have many hooks, but there are 2 service parameters that can tell Drupal 11 to stop procedural hook scan and these are purely optional: Improve performance by preventing unnecessary scanning of procedural hooks → and hooks_converted parameter and StopProceduralHookScan attributes have been renamed. → . Example if you wish to add them:
parameters: # For Drupal 11.1 and lower. MODULENAME.hooks_converted: true # For Drupal 11.2 and higher. MODULENAME.skip_procedural_hook_scan: true
- The error message passed to the addError method is not translatable in src/Hook/ParagroupHooks.php line 128.
- The create method should return new static() instead of new self() in src/Form/ParagroupConfigForm.php
- src/Hook/ParagroupHooks.php: The $helper and $field_group_manager arguments are missing their type causing the autowire service to fail. Also, the @var \Drupal\paragraph_group\Paragroup\ParagroupHelper class doesn't seem to exist anymore (line 31).
- The validateForm is a method that returns void. In the src/Form/ParagroupConfigForm.php returning a void function result is incorrect.
- There is no entity type that has the paragraph_type ID: $this->entityTypeManager->getStorage('paragraph_type')->loadMultiple(); in src/Paragroup/ParagroupFormData.php
Actually, in its current state, the module throws fatal errors in both Drupal 10 and 11, it is unusable. Please ensure that the module is running smoothly with Drupal Core ^10.2 || 11 as described in the info file, and carefully test every aspect of it and/or provide tests to ensure quality and stability.
Hi! Quick note: you don't need to tag a new release after every feedback. We're reviewing directly the branch specified in the title of the ticket, in this case 1.4.x.
It is better not to create new releases during these applications, since a review could ask for a change that is not backward compatible with the existing releases. Just using a development version avoids those BC issues.
Hi! Below you have my feedback:
- There are still 2 pending DrupalPractice warnings:
FILE: /var/www/html/web/modules/contrib/ai_log_analysis/ai_log_analysis.module ------------------------------------------------------------------------------ FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE ------------------------------------------------------------------------------ 14 | WARNING | Unused variable $max_logs_per_type. ------------------------------------------------------------------------------ FILE: /var/www/html/web/modules/contrib/ai_log_analysis/ai_log_analysis.routing.yml ------------------------------------------------------------------------------------------------------------------ FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE ------------------------------------------------------------------------------------------------------------------ 15 | WARNING | Open page callback found, please add a comment before the line why there is no access restriction ------------------------------------------------------------------------------------------------------------------
- ai_log_analysis.info.yml: The module should be compatible with ^10.2 || ^11, since the ai module dependency's minimum Core requirement is 10.2. Is there a reason why this module is not yet compatible with Drupal 11?
- The dependencies in ai_log_analysis.info.yml are prefixed with drupal: when the module is coming from core, otherwise they're prefixed with their own module name. For example, the ai module that is a dependency to your module is a contrib module, therefore the dependency should be ai:ai
- composer.json should probably specify license "license": "GPL-2.0-or-later" and you don't need to require the "drupal/core": "^10" package. Would be nice to include support information as well. See
Add a composer.json file →
for more info.
- ai_log_analysis.install: I'm not sure I understand the necessity of the ai_log_analysis_install() hook and what table is custom_log. It does not match with the description of the function, is it still needed?
- ai_log_analysis.module: - For a new module that aims to be compatible with Drupal 10 and Drupal 11 (assuming that the core requirement will be changed), I would rather implement hooks as class methods as described in
Support for object oriented hook implementations using autowired services →
.
- Fix the open page callback in ai_log_analysis.routing.yml also reported by PHPCS
- Fix the README.md file. The markdown is a bit broken
- src/Command/ErrorLogAnalyzerCommand.php:
--- I'd convert the definition of the Drush command from PHP Annotations to PHP Attributes as shown in Creating Custom Commands.
--- I'd use $this->io() to interact with the output, especially for questions. See The io() system
- src/Form/LogAnalysisSettingsForm.php: With Drupal 10 and Drupal 11, there is no longer need to use #default_value for each form element, when the parent class is ConfigFormBase: It is sufficient to use #config_target, as in the following code:
$form['image_toolkit'] = [
'#type' => 'radios',
'#title' => $this->t('Select an image processing toolkit'),
'#config_target' => 'system.image:toolkit',
'#options' => [],
];
- src/Logger/AiLogAnalyzerLogger.php: should probably use str_contains instead of strpos
🙌