For anyone coming to this issue with the same problem I confirm that requrie-dev trick solved the issue.
Thanks!
IT seems there's a workaround: https://www.drupal.org/project/gitlab_templates/issues/3518843#comment-1... 🐛 Dependencies in submodules are not taken into account Active .
No, no module had o tests before, not in Drupal CI but also not in Gitlab CI.
The require-dev seems like a reasonable way to solve this in a simple way without increasing complexity in the GitLab CI testing workflow, I'll try that, thanks!
It seems the problem with phpstan is a bug in the Gitlab CI templates, see 🐛 Dependencies in submodules are not taken into account Active .
There's an error from phpstan tan I can't fix.
The problem is:
------ ------------------------------------------------------------------------
Line modules/image_styles_generator_webp/src/DerivativeWebpWarmer.php
------ ------------------------------------------------------------------------
31 Parameter $webp of method
Drupal\image_styles_generator_webp\DerivativeWebpWarmer::__construct()
has invalid type Drupal\webp\Webp.
------ ------------------------------------------------------------------------
And in line 31 we can see:
public function __construct(LoggerInterface $logger, Webp $webp) {
We can expand the header:
/**
* Constructs a Regenerator object.
*
* @param \Psr\Log\LoggerInterface $logger
* The logger channel to log all info.
* @param \Drupal\webp\Webp $webp
* The webp service.
*/
public function __construct(LoggerInterface $logger, Webp $webp) {
And check that at the beginning of the file there's a use statement.
use Drupal\webp\Webp;
The Webp class if from another module (the Webp module) but it is declared in the dependencies of the image_styles_generator_webp submodule.
phpstan errors and phpcs errors are standard code errors and can be easily fixed.
However, cpsell is more tricky. The typos must be fixed, but words like "commandfile" are legit so we have to instruct cspell to ginore certain words that are not typos.
For this, we need to create a .cspell.json file (see https://project.pages.drupalcode.org/gitlab_templates/jobs/cspell/) with this content.
variables:
_CSPELL_WORDS: 'commandfile'
I've changed the option from fast to skip-existing, apart from that the patch was good, thanks!
This is indeed very interesting!
Let me review it and come back with my findings.
Remove comments on Responsive Thumbnail Image as there's a D11 release available now
I guess this will be fixed when 🐛 Svg images does not render when applying image style Needs work lands into core.
While that happens, patchs are welcome.
This module uses the settings form of ResponsiveImageFormatter (it just extends this class and calls parent's settingsForm). This should fixed in core. This way, all image formatters will benefit from the fix.
See https://www.drupal.org/project/drupal/issues/1291262 ✨ Add 'alt' and 'title' tokenized text options for image formatters, and a 'title' option for the generic file formatter Active .
There's a new 2.x branch where this patch is going to be merged. We'll have a Drupal 11 release and switch to semver.
Hi glynster, thanks for the report and the patch!
I would like to reproduce the bug, can you provide the steps to reproduce it?
Can you provide more info on this issue?
Please feel free to reopen if there's a way to replicate the issue.
Thanks!
No answer from maintainer.
Moved to "Drupal.org project ownership".
Module: https://www.drupal.org/project/media_responsive_thumbnail →
Thanks, it looks good to me!
I would prefer to wait for comments from @lpeidro before committing this forward but I think it is ok to move it to RTBC.
Tested, it seems to work ok, available in release 2.1.0 already deployed.
I missed this one.
Good to know the root cause, thanks for sharing!
BAck to "No known problem" as the issue the moved this page to "Needs work" has been addressed.
"Recommended" releases have been removed per 📌 Remove “recommended” status for releases? Active but this page still talks about them.
With this change this page is outdated: https://www.drupal.org/docs/develop/git/git-for-drupal-project-maintaine... →
It still references "recommended" releases. I'm editing the page to indicate the problem, but I'm not confident enough to edit it.
Created 2.0.0 release!
The "core" property in the info.yml needs to be removed. This means that module is not compatible with Drupal 8.7 and previous (see Let Drupal know about your module with an .info.yml file → ).
Hence, we need to drop support for 8.7 and before. This implies creating a 2.0.0 release. I don't like it for such as small change, but SemVer dictates that breaking changes implies a major version, and this is a breaking change.
An example of the view:
I stumbled upon this issue again. I've tried to better explain the situation to address #28:
Note that if #empty_value is the same as a key in #options then the value of #empty_option is used for that key in the #options array. This is because #empty_value and #empty_option are merged into the #options array. Hence, make sure #empty_value is not a key in #options array.
If this is not enough, please point out what is missing or what would be a better approach to this. The issue is complex and it is hard to understand without a little bit of context or checking all the comments in the class' header.
I'm assuming you are using Content Moderation and the default "Editorial" workflow. In that case, it should work. Just as reference, this is the test I've done in my local:
- Enable Content Moderation module
- Apply "Editorial" workflow to "Basic Page" content type (/admin/config/workflow/workflows/manage/editorial)
- Create a new Basic Page and save it as draft
- Create and authlink for that content
The generated authlink works and the unpublished draft is visible.
If this is not the case for you recheck the workflow configuration, or detail your conf so we can reproduce your problem.
Added some minor tweaks to the view.
Thanks!
I would like to propose a border or similar to mark releases that are covered by the security advisory policy.
Something like this:
This may also help to distinguish between stable and not stable releases (as long as the module has opted-in the security advisory policy).
It looks good to me, but I'll wait for @lpeidro review.
Thanks!
Also, I guess that process can be done directly using SQL. I mean writing a SQL query that returns what the values calculated in $main_fid and $unused_fids. Probably, this is not trivial, but if done will solve many performance problemas with sites that have huge number of files.
I don't know of such tool, I wanted to have the WID just to locate the original message easily.
Thanks for the quick response!
Define the alyout for each node o use a fixed one for all.
Now we have the shuttle and the demo modules, let's explain this.
Add info about Media Responsive Thumbnail and Media Responsive Thumbnail.
Yes, you are right Drupal 11 is not available, I was focusing in the issue description and I totally forgot D11.
The issue seems active then.
This was fixed in 2.0.1. release.
Let the bot do its job.
Added suggested patch.
Mm... I've checked and Drupal core 10.4.4 is available in Simplytest. However, the module release I pointed out is not.
I think this can be closed as outdated.
It also happens with module releases. For example, for Monitoring module the last release available is 1.14 while last release is 1.17.
Added the wid directly to the message.
I would wrap the id tihe '[]', but because the other fields are printed without any separators or wrappers I just added it naked. If output format is not fixed (because people tends to process it someway) I would separate fields somehow (for human consumption more than computer comsuption).
I've tried patch from #23 and but see no improvement, the content is displayed totally broken (CSS issues). Not sure if this is because the use of layout builder, paragraphs or what. Using Drupal 10.4.4.
@jez300 that's indeed an strange behavior. I think that should be an issue, because I don't see any reason to restrict the form id in such way. However, from your message, it is not clear if the form ID should not match the "twig template name" (as stated by you in your message) or the theme name (as stated in your edition). Can you clarify that? An can you create the issue?
I'm not sure if this is something really needed, but it was suggested from some feedback on the theme. I think is worth to write it here.
If you want this functionality, please comment and add your point of view, thanks!
Hi kopeboy, thanks for your interest.
The module doesn't generate a file but an inline SVG, see screenshot.
This means that it will be cached depending on where it is placed, the module doesn't provide any caching configuration, so it uses standard Drupal field formatter caching.
Marked as fixed, fell free to reopen it if this response is not clear.
As I'm Form API guide maintainer I'm removing it from the list that need maintainer.
Thanks bot, proposed patch merged.
Fixed in dev, thanks!
I'm not able to reproduce this bug. I've tried using node 23.4.0 but it compiles without errors. I'm using DDEV and Aljibe (https://github.com/Metadrop/ddev-aljibe/), but anyway it should work the same with any environment as long as the Artisan release and node are similar.
Can you try using Aljibe and/or DDEV? To discard problems on your local setup.
Merged and included in release 2.1.0.
Release 2.1.0 available, now compatible with Drupal 11.
Solved conflicts in the bot branch and merged to 2.0.x
Thanks for the comments, let me have a look into it and come back with results.
Thanks for coming back and close the issue!
If you track down the mentioned issues to this module don't hesitate to open a new one.
It seems original node was removed. I can't find a new node with the same information. Anyone knows a good link to add there?
I've seen this one in Drupal Answer but not sure if it is a good fit: https://drupal.stackexchange.com/a/32645
Now it's fine, the articles are in Spanish and the blog URL is displayed OK (I guess is taken from the downloaded RSS).
Thanks for the quick response!
I would like to add my own thoughts:
- Relying on Composer to provide 1 and 2 is tricky because Composer doesn't cover this exact use case so it feels like we could develop a clever solution instead of an engineer solution. This will bite us back in the future
- Relying on data from Drupal.org feels better because we are proposing a funcionality on top of composer and php dependencies, on an upper level.
A practical proposal (but keep in mind that know PB just a user, not dev, I might be totally wrong):
- Recipes's yml file allows to declare suggested (and thus curated) optional recipes to apply
- Recipe's yml file allows to declare itself as suggested (and thus, not curated) optional recipes to apply given the parent recipe is being applied.
- With this info, given a recipe PB can display a list of suggested recipes (curated ones and not curated)
- When applying recipes, PB downloads all selected recipes and runs them. This way, only actual used dependencies are downloaded
- Each recipe is a Composer package
- In the future, depending on infra, recipes could be managed in a monorepo
I've read the issue and the Slack chat and I would like to summarize (I hope I got everything right).
We are touching several requeriments here:
- (1) Allow Recipes to declare suggested recipes (optional recipes that expand the recipe's functionality) (#0, #10)
- (2) Allow recipes to declare dependencies on other recipes (so they declare they expand another recipe's functionality), so they are 'available' when you apply the recipe they are dependent on (#10)
- (3) Allow PB to offer users related recipes based on suggested and available
From there, some problems/topics arise:
- (A) How Recipes declare related recipes in such way PB and composer get the info
- (B) How to manage recipes in terms of Composer packages to avoid issues like a snowball effect on recipe dependencies (recipe A suggests/recommends B, C, ..., and recipes M, N. O, etc, depend on A) so a site is not flooded with code from dependencies that are not used at all (#7)
- (C) How to manage the code of recipes in git terms: one repo per Composer package, a monorepo with subtree split with different composer packages ([#5, #6, #7]). This is less relevant as from a functional point of view as they behave the same (they are just different packages (#9))
The possible solutions (from #7):
- (S1) Rely on composer dependencies to install recipes and its dependencies and use that info in PB to offer related recipes (PB can guess the parent<->child relation of recipes, see also #10). This solution will hit the snowball effect mentioned above.
- (S2) Rely on D.org API that reports the relation of recipes and let PB display the related recipes. On installation, PB would composer require all the required recipes.
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.