🇪🇸Spain @tunic

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!

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

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.

tunic → created an issue.

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.

Not known problems.

Now we have the shuttle and the demo modules, let's explain this.

Add reference to Artisan as the recommended theme.

No known issues.

Add info about Media Responsive Thumbnail and Media Responsive Thumbnail.

tunic → created an issue.

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).

tunic → created an issue.

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!

tunic → created an issue.

tunic → created an issue.

tunic → created an issue.

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.

tunic → created an issue.

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!

tunic → created an issue.

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.

Add installation section.

Small fixes in current doc

WIP notice

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 → .

Adding lines between content paragraphs.

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.

tunic → created an issue.

tunic → created an issue.

tunic → created an issue.

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.

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/

tunic → created an issue.

tunic → created an issue.

tunic → created an issue.

tunic → created an issue.