🇩🇪Germany @J-Lee

Same issue here. A larger project fills the log with this error message.

Regarding #3382665-19: Replace Drupal login with Keycloak single sign-on (SSO) is not working → , I think this issue is outdated.

After some investigation, it looks like that 🐛 LogicException when redirecting to Keycloak Needs review is not needed. The MR calls $request->send() from there, which is done by Drupal itself.

Attach interdiff between #7 and #17.

I have added the changes from #15 to the patch from #7. It works with the latest 2.2.x branch.

Now, the next error appears: "Failed to start the session because headers have already been sent by "/app/vendor/symfony/http-foundation/Response.php" at line 1315."

I have fixed #14 by changing Drupal\keycloak\Controller\KeycloakController::login() as following:

public function login() {
    $this->session->saveDestination();
    $client_name = 'keycloak';

    $config = $this->config('openid_connect.client.keycloak');
    $settings = $config->get('settings');
    $pluginCollection = new OpenIDConnectClientCollection($this->pluginManager, $client_name, $settings, $config->get('id'));
    $client = $pluginCollection->get($client_name);

    $scopes = $this->claims->getScopes();
    $_SESSION['openid_connect_op'] = 'login';
    return $client->authorize($scopes);
  }

With this change, the client_id is set and the redirect is working.

Works with applying the patch form #7 and 🐛 LogicException when redirecting to Keycloak Needs review (#8)

j-lee → created an issue.

I have left a suggestion so that someone with write permission can apply it directly to MR.

An other version, wich reduce the calls from getSettings().

Patch with the check for NULL.

j-lee → created an issue.

The patch from #33 also works for us.

Test coverage is missing, so leave it at needs work

The MR seems to be empty.

The tests use \Drupal\Core\Field\BaseFieldDefinition, which returns a non-unique identifier for the same field name.

Did someone have a suggestion for the best way to test this?

However, it is theoretically possible for $this->fieldDefinition->getUniqueIdentifier() to return an empty string.

Reroll against latest dev.

Found another problem with the non-URL options. I have created a MR and hidden the deprecated patch.

I also added a small improvement with the setDirective() method on the CspPolicy event subscriber. This is now called once after processing the domains.

This was asked some time ago in #3214847: Allow entity references in custom composite elements → .
But I think it's a good idea to make e.g. nodes selectable for some kind of product selector or whatever.

Great. Thank you.

Attached is a patch which fixes the issue.
Maybe the "Add Google supported domains" checkbox should not added to non-URL fields.

J-Lee → created an issue.

J-Lee → created an issue.

J-Lee → created an issue.

Sounds like a duplicate of 🐛 Experimental widget displays the original language on translation Needs work .

Have added the core question as related so that they are linked.

@piridium can you please add your changes to the merge request?

@james.williams have found two examples for the wording:

@trigger_error("The property $name ($service_name service) is deprecated in $class_name and will be removed before Drupal 11.0.0.", E_USER_DEPRECATED);

@trigger_error('The key-value pair "experimental: true" is deprecated in drupal:10.1.0 and will be removed before drupal:11.0.0. Use the key-value pair "lifecycle: experimental" instead. See https://www.drupal.org/node/3263585 → ', E_USER_DEPRECATED);

Maybe it helps?

I think, 📌 Add keycloak support for multple instances from openid_connect 3.x Needs review should be part of this.

I noticed the same problem when I tried a Keykloak implementation. The MR looks good so far in a quick review.

I think, we can move it to the Drupal.org project ownership issue queue.

J-Lee → created an issue.

@Anybody have you received a response from the project owner/maintainer?

From How to become project owner, maintainer, or co-maintainer → :

If the owner does not reply after two weeks, move the issue to the Drupal.org project ownership issue queue, by changing the issue's Project field to Drupal.org project ownership. Please include a link to the project page.
Site moderators will normally try to contact the owner as an extra safeguard unless it is evident the owner is no longer active in the Drupal community, giving to the contacted users two weeks for replying to the message they sent via the Contact tab.

I think I fixed the new tests for the default stylesheet behavior.
As a followup, perhaps a test base class could be created to better organize the tests.

Sorry for the long wait but I had to fix my dev environment first. Thanks @Anybody for the link.

I have problems with the local test environment and a chmod permission problem, so I will test here.

Working on the tests

Opened the MR

J-Lee → made their first commit to this issue’s fork.

I can confirm, that it works well. Thank you.

@joco_sp Can you confirm that the script has loaded? Maybe it can also help to include some console.log() for debugging.
In most cases, a caching mechanism is responsible for making it appear that it is not working. Clearing Drupal caches and possibly also the browser cache usually helps.

@Asterovim I cannot confirm your problem. TagManager loads and is visible. I have also used Google's Chrome extension "Tag Assistant" to troubleshoot.
Are there any error messages?
Have you tried clearing the cache?

Looks good to me.

Same question here.

Which Version should we use (8.x-1.x or 2.0.0) for new projects or updating to Drupal 10?
What are the difference between the two versions? Removed deprecated code from Drupal 8 and 9 or new features? Both has support for Drupal 10.

Thank you.

I have missed this configuration :(
If I set it to something else, the issue is fixed.

I am not sure about the type "text". Is it allowed to use it multiple times somewhere?

A quick test with adding "text" to neverExplode() and with the following ruleset at Drupal\schema_metatag\Plugin\metatag\Tag\SchemaNameBase::processItem() fixes the problem:

    switch (true) {
      case $key === 0:
        $explode = $this->multiple();
        break;
      case in_array($key, $this->neverExplode()):
        $explode = FALSE;
        break;
      case !in_array($key, $this->neverExplode()):
        $explode = TRUE;
        break;
      case $this->schemaMetatagManager->hasSeparator():
        $explode = TRUE;
        break;
      default:
        $explode = FALSE;
    }

I'm not sure I understand it correctly.

But shouldn't the text property also appear in Drupal\schema_metatag\Plugin\metatag\Tag\SchemaNameBase::neverExplode()? And then wouldn't the order of the $explode query in Drupal\schema_metatag\Plugin\metatag\Tag\SchemaNameBase::::processItem() have to be adjusted and checked first for neverExplode() and then for hasSeparator?

    if ($key === 0) {
      $explode = $this->multiple();
    }
    elseif ($this->schemaMetatagManager->hasSeparator()) {
      $explode = TRUE;
    }
    else {
      $explode = !in_array($key, $this->neverExplode());
    }

becomes

    if ($key === 0) {
      $explode = $this->multiple();
    }
    elseif (!in_array($key, $this->neverExplode()) {
      $explode = TRUE;
    }
    else {
      $explode = $this->schemaMetatagManager->hasSeparator();
    }

I have to revise my statement. It is only commas and not br tags. Sorry for the disturbance.

It looks like this, that even with commas in the text the property is split up

We have a WYSIWYG editor (v5) text field containing a text with multiple breaks (br tag). The text is then split in the schema at the br-tags and for each br-tag a text property is inserted.

The problem persists with Metatag 2.0 and Schema Metatag 3.0.1

I addressed the points from #9 and fixed a few minor bugs.
With this the MR is ready to be reviewed further.

1. I can add something like /** @group google_tag_v1 */ and /** @group google_tag_v2*/.
2. The scripts must be executed for the Tag Manager to load. However, the scripts are IIFEs. I can do something like this:

...
const script = document.querySelector('script#' + id);
const code = script.textContent || script.innerText;
const executeCode = Function(code);
executeCode();

But, is this more secure?

Compatibility for google_tag module v1 restored.
The ERR_SOCKET_NOT_CONNECTED error was apparently only a local problem and cannot be reproduced at the moment.

The tests should be fine for the google_tag module v2. Since the functionality of JS is actually the same.

The MR is not ready yet.

• The support for the google_tag module v1 still needs to be checked.
• There are errors when loading the Google Tag Manager: Failed to load resource: net::ERR_SOCKET_NOT_CONNECTED for www.googletagmanager.com/gtm.js?id=GTM-XXXXXX

After looking at the new behavior, I'm not quite sure which way to go.

The iframe is inserted via hook_page_top() (is now rendered via a template).
Via hook_page_attachments() the information about the Google tag manager (gtm) and Google tag (gtag) are inserted in drupalSettings. For both, a JS-library is also added there. In the JavaScripts the information is read from drupalSettings and the script tags are generated dynamically.

As far as I understood, so far (google_tag module v1) the script type="text/javascript" was changed to script type="text/plain". After the approval was successful, the text/plain was then removed again, which enabled the script.

In version 2 the script tags are no longer generated in PHP. Therefore, the only option I see right now is to use hook_page_attachments_alter() to remove the two libraries (gtm and gtag) and rebuild the two scripts in the cookies module and adjust them accordingly so that it matches the current behavior

Google tag manager script: https://git.drupalcode.org/project/google_tag/-/blob/2.0.x/js/gtm.js
Google tag script: https://git.drupalcode.org/project/google_tag/-/blob/2.0.x/js/gtag.js

J-Lee → created an issue.

The nit-pick could be done on commit.

Thank you all for the work.

Strange that sometimes my suggestion is needed and sometimes not. Maybe it is due to different language settings?
To make the codebase more robust, I would suggest to apply the check to the language/translation.

Attached the new patch file.

I can confirm the problem and have left a comment at the appropriate place in MR. With $entity->hasTranslation($this->langcode) and $entity->addTranslation($this->langcode) we can solve this.

I cannot change the target branch at the MR to the 2.0 branch 😞

The points from #58 are not relevant here.

But I think we should split this topic into several points because there are different functions.
The original request was that Colorbox should be compatible with a responsive image formatter. We should focus on that and merge that first.

Another feature that doesn't work (cleanly) is to display a responsive image inside Colorbox. We should address this in a separate followup issue.

Perhaps the maintainers can comment briefly on the strategy?

I have updated MR to #46 and fixed #51 and #56.

But there are more issues e.g.:

• $colorbox_style = $config->get('colorbox_style') But there is no config 'colorbox_style'. Maybe it should 'custom.sytle'
• Same with $config->get('colorbox_caption_trim_length') -> Should be 'advanced.caption_trim_length'
• ... there are more ...

In summary, we should continue with patch #46 ✨ Support Responsive Image module in D8 Needs work and respect the reviews from #50 ✨ Support Responsive Image module in D8 Needs work , #51 ✨ Support Responsive Image module in D8 Needs work and #56 ✨ Support Responsive Image module in D8 Needs work .

@@ -139,6 +82,190 @@ function template_preprocess_colorbox_formatter(&$variables) {
     $variables['url'] = $file_url_generator->generateAbsoluteString($image_uri);
   }

+  if (!empty($variables['responsive_image'])) {
+    $attributes = [];
+    // Do not output an empty 'title' attribute.
+    if (mb_strlen($item->title) != 0) {
+      $variables['responsive_image']['#title'] = $item->title;
+      $data_cbox_img_attrs['title'] = '"title":"' . $item->title . '"';

$item->title could be NULL, so a deprecation will be triggert: "Deprecated function: mb_strlen(): Passing null to parameter #1 ($string) of type string is deprecated". Maybe change to if (mb_strlen($item->title ?? '') != 0) {

The rector patch from #3 looks good to me.

MR7 and Patch #9 are identical.

Patches from bot looks good.

The changes to the info files could be done on commit:
contact_permissions.info.yml
tests/modules/contact_permissions_test/contact_permissions_test.info.yml

Value of core_version_requirement: ^8 || ^9 is not compatible with the next major version of Drupal core. See https://drupal.org/node/3070687.

Ah, there are still the tests missing ... back to needs work.

Looks good. Thank you.

In the LayoutParagraphsLayout class, there is a getEntity() method that gets the entity from the entity reference field, but there is also a setEntity() method that sets the entity dynamically and is not used. This looks a bit strange. Maybe this is the point where you could start.

/**
   * Returns the layout's parent entity with updated paragraphs reference field.
   *
   * @return \Drupal\Core\Entity\EntityInterface
   *   The entity.
   */
  public function getEntity() {
    $entity = $this->paragraphsReferenceField->getEntity();
    return $entity;
  }

  /**
   * Set the entity that this layout is attached to.
   *
   * @param \Drupal\Core\Entity\EntityInterface $entity
   *   The entity to set.
   *
   * @return $this
   */
  public function setEntity(EntityInterface $entity) {
    $this->entity = $entity;
    return $this;
  }

@joachim You are right. It doesn't seem to be possible to get the currently used language from the field.
Either the entity must be specified or the language, which in turn would have to be taken into account in LayoutParagraphsLayout.

Maybe because the entity is cloned at buildEntity()?

I think justin2pin closed the MR because of the many changes since the MR was created. That is why the MR is out of date.

The patch from #14 changes the field values to the translated values. But the field with the source language is inserted into the LayoutParagraphsLayout (Line 129 in patched Drupal\layout_paragraphs\Form\LayoutParagraphsBuilderForm) which is used to create the tempstore key. If the field language is changed, the correct language will be displayed in the UI after saving the changes.

      if ($entity->hasField($default_langcode_key)) {
        $entity = $entity->getTranslation($langcode);
+      $entity->$field_name->setLangcode($langcode);
        foreach ($entity->$field_name as $delta => $item) {

A quick and dirty fix:

\Drupal\responsive_image_preload\PreloadGenerator::generatePreloadsForDelta() #144:

case 'image_style':
              $image_style_name = $image_style_mapping['image_mapping'];
              /** @var \Drupal\image\ImageStyleInterface $image_style */
              $image_style = $image_style_storage->load($image_style_name);
              $url = $image_style ? $image_style->buildUrl($file->getFileUri()) : $file->getFileUri();
              $image_url = $this->fileUrlGenerator->generateString($url);

J-Lee → created an issue.

Drupal core issue for it: 🐛 form_build_id is a hidden input field and therefore should not have the autocomplete attribute Needs work

I think that's done, isn't it?

There is another deprecation in src/Plugin/StyleOption/BackgroundImage.phpsrc/Plugin/StyleOption/BackgroundImage.php at line 40.

Call to deprecated method toInt() of class Drupal\Component\Utility\Bytes. Deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use Drupal\Component\Utility\Bytes::toNumber() instead

J-Lee → created an issue.

Looks good to me.

The patch needs a reroll against the latest dev.