🇩🇪Germany @stefan.korn

stefan.korn → created an issue.

Good idea.

What do we need?

• config schema
• Global setting form (where to place)
• Change in BreadcrumbBuilder logic based on this setting
• Disable the Third Party Setting for individual vocabularies and give a hint if global setting is active.

Will try to implement this soon.

I was also hit by this change, in that we are not using the assumed role name "administrator" for the admin role.

@eiriksm pointed out, a role called administrator does not necessarily exist nor need it to be an admin role.

Proposing to change the check for an admin user like this:

diff --git a/src/AdministratorTrait.php b/src/AdministratorTrait.php
index 60821e8..5f5128a 100644
--- a/src/AdministratorTrait.php
+++ b/src/AdministratorTrait.php
@@ -25,9 +25,18 @@ trait AdministratorTrait {
     static $root_user = NULL;
 
     if (!$root_user) {
+      $admin_roles = $this->entityTypeManager()->getStorage('user_role')->loadByProperties(
+        [
+          'is_admin' => TRUE
+        ]
+      );
+      $admin_role = reset($admin_roles);
+      if (!$admin_role) {
+        throw new \RuntimeException('No admin role found');
+      }
       $query = $this->entityTypeManager()->getStorage('user')->getQuery();
       $ids = $query->condition('status', 1)
-        ->condition('roles', 'administrator')
+        ->condition('roles', $admin_role->id())
         ->accessCheck(FALSE)
         ->execute();
       $users = User::loadMultiple($ids);

Would you consider to change this? I can open a new issue with MR in that case.

Proposal by eiriksm to change to previous behavior and load user 1 if no admin user is found, might also be an option.

Regarding tests I am not too proficient. I would maybe need some input on this. I guess I fixed on of the failing tests, but probably one would have the tests not only fixed, but also targeting the new setting.

Kindly ask for review.

stefan.korn → created an issue.

Yes it's true, the latest changes were not reflected in the module description and README. I have updated README and module description now.

Thanks for pointing this out.

see MR for a possible fix.

So far I could not spot any problems or changes in functionality of the toggler, but the warnings and notices are now gone.

stefan.korn → created an issue.

Thanks for testing and reporting deprecation notice, should surely not give this warning.

I have committed a change in the dev branch.

stefan.korn → created an issue.

I am using this for a while now, so setting to "Needs review"

Released version 1.3 to fix this.

Hook for adding formatters, like so:

function hook_media_parent_entity_link_alter_formatters(&$formatters) {
  $formatters[] = 'blazy';
}

works with blazy as far as I can tell.

Feature is now in dev branch.

stefan.korn → created an issue.

@erwangel: thanks for reporting.

I suppose the issue was introduced with ✨ It works with private images, not working with thumbnails RTBC , thus it will come with version 1.2.

I have committed a fix to dev branch. Can you test with dev branch?

I will make a new release soon, as the issue is clear in limiting media parent entity link to image formatter only in version 1.2.

I did probably not test ✨ It works with private images, not working with thumbnails RTBC with responsive image formatter.

@fenstrat: Thanks for your input.

Regarding the media type styling I was unsure. Claro uses vertical tabs:

Bootstrap provides this for vertical nav:
https://getbootstrap.com/docs/5.3/components/navs-tabs/#vertical
But imho this is looking a bit to "sleek", especially there is no visual hint regarding the active state. So I decided to use vertical pills, like shown at the end of this paragraph in the Bootstrap documentation: https://getbootstrap.com/docs/5.3/components/navs-tabs/#javascript-behavior

It seems Bootstrap is currently not providing something exactly similar with its defaults to what Claro is using.

I confirm the issue and the MR resolves this issue.

Setting to RTBC.

stefan.korn → created an issue.

I did also notice similar problem with locked submission in webform 6.2.x. The process is as follows:

- Create a webform
- add an action that locks webform after submission
- use token in the confirmation text

The token is not evaluated in the confirmation page if the locking action is present, otherwise the token is evaluated.

It also depends on confirmation type as it seems:

It works if confirmation type is set to
- Message
- URL with message

It does not work with confirmation type set to
- Page
- Inline

Okay, seems like one can override the ckeditor5-styles from varbase_components module as follows:

define a library in your_theme.libraries.yml as follows

ckeditor5-styles:
  dependencies:
    - core/components.your_theme_components--root
    - core/components.your_theme_components--alert
    - core/components.your_theme_components--callout

and then in your_theme.info.yml add this to libraries_override:
varbase_components/ckeditor5-styles: your_theme/ckeditor5-styles

not sure if this is already pointed out somewhere.

I think there is an issue with this, when trying to override the root component ✨ Add a Root component as a Base, Contains root CSS3 Bootstrap variables to Varbase Components Fixed in your theme. The ckeditor5-styles do not respect the override and you cannot easily override ckeditor5-styles dependencies in your custom theme yourself ✨ Allow overriding/disabling base theme's ckeditor5-stylesheets value Active (I am not even sure that the workaround given there does actually work. did not get it to work at least).

So if the ckeditor5-styles dependencies are called you get the root.css from the theme and from varbase_components and the order of appearance does matter. In my case the root.css from varbase_components came after the one from the theme. And anyway it does not look correct that both are called.

starterkit was probably added with #3050384: Provide a starterkit theme in core → . Add I think the issue with description display was fixed with #2396145: Option #description_display for form element fieldset is not changing anything → , but that did not find its way back in starterkit supposedly.

Question is maybe why the fieldset template (and some other templates too) are in the starterkit at all, when they maybe could be delivered from base theme stable9 as well?

I created core issue 🐛 starterkit theme should use same fieldset template than stable9 Active

stefan.korn → created an issue.

@jannakha: Please see screenshots

Claro:

Bootstrap5 without this MR (notice both Save and Preview are primary):

Bootstrap5 with this MR (Preview now Secondary):

This will possibly conflict with some other MRs I have created, because of using preprocess_form_element and preprocess_input in these MRs too. If acceptable the MRs would need to be processed on after another and need to be rerolled.

Screenshot of regular textfield and textfield with floating label:

Screenshot of regular textfield and textfield with floating label in focus:

Code is

    $form['text'] = [
      '#type' => 'textfield',
      '#title' => 'Textfield',
    ];

    $form['text_float'] = [
      '#type' => 'textfield',
      '#title' => 'Textfield',
      '#attributes' => [
        'placeholder' => 'Password',
        'floating_label' => 1
      ]
    ];

stefan.korn → created an issue.

added styling for media type tabs

screenshot:

stefan.korn → created an issue.

stefan.korn → created an issue.

Regarding Task #1 I copied over the whole variable twig comment from stable9. This has some additional small changes beside adding description_display part.

see updated MR

@jannakha: thanks for reviewing this.

Regarding css/components/form.css: We could do the changes instead in scss/drupal/_forms.scss, but this does have some caveats too I suppose. One would need to overwrite the CSS from form.css in forms.scss, maybe something like margin-left: inherit;. While this is possible, seems not to be the cleanest way.

In addition this would somehow still depend on form.css. If for example the CSS selectors are changed in form.css, the fix in forms.scss might not apply anymore. This is kind of a hidden dependency.

So maybe think about if you might not still prefer the way via form.css. Maybe one could add a line with comment like "removed unnecessary margin for description" and then if you incorporate upstream changes from starterkit you would see this line in a git diff and can keep track of the change this way. This would of course not work if you incorporate the upstream changes automatically without reviewing. In that case we would probably need to do it elsewhere like in forms.scss

@jannakha: True, that issue exists in the starterkit theme as well. Did not expect that, though I spotted a difference in form-element template template in 💬 form element description does not get description class if description display is set to before Active already.

Wondering why starterkit uses different template from its base theme stable9 at all here ...

Okay, I think there have been some unrelated changes to CSS in the MR introduced by changing/updating the node packages.

I think, this should not be part of this issue, but in case it is needed be solved in a separate issue.

There were some really strange changes to the CSS like

--bs-primary-text-emphasis: rgb(5.2, 44, 101.2);

This results from latest changes in SASS, see https://sass-lang.com/documentation/breaking-changes/color-functions/ and using 1.79.x for sass would require changes in handling colors. But this is surely out of scope for this issue. More a general build issue for bootstrap5 theme.

So imho the build process should be done with sass version 1.54.0 as current package-lock.json states.

I therefore did the build process with sass 1.54.0 and reverted changes that were introduced by building with newer sass version (1.79.1).

I can confirm the issue and the MR is greatly helping to fix this.

I even think that the modal size and tab styling is okay now.

stefan.korn → created an issue.

No offend to maintainers, but this is a perfect example where the ever growing number of (probably mostly automated) "code style" fixings get worse by breaking an essential function of the module. I am a module maintainer myself and imho it's like kind of spam nowadays how these issues are created at mass without having real value in improving the module.

And specifically the rule "All functions defined in a module file must be prefixed with the module's name" has potential for trouble causing BC breaks. This should not be lightly used in such automated fixes.

And once again, full respect and thanks to maintainers of devel_debug_log, I use it very often and it is very helpful.

stefan.korn → changed the visibility of the branch 3474980-align-default-button to hidden.

stefan.korn → created an issue.

@cilefen: Thanks for quickly looking into this. 🐛 Prefix is shown before description Active is not touching the related part (see this vs that in issue description), so it would not solve this issue and thus is probably not related, beside that it is touching the same template.

stefan.korn → created an issue. See original summary → .

stefan.korn → changed the visibility of the branch 3474642-fieldset-template-should to hidden.

stefan.korn → created an issue.

The action plugin has been moved to Action module in Drupal 10.3, see https://www.drupal.org/node/3424506 →

this patch should apply to 10.3

that said, for Drupal 11 the patch needs to be ported to the then separated action module: https://www.drupal.org/project/action →

committed a change that adds support for YT shorts.

been using dev version in a project for a while, therefore settings this to RTBC

been using dev version in a project for while, therefore setting this to RTBC

testing a bit, it seems to me that just removing this return statement solves the issue.

      if (($source_entity instanceof RevisionableInterface) &&
        $source_entity->getRevisionId() != $source_entity->original->getRevisionId() &&
        $source_entity->hasField($field_name) &&
        !$source_entity->{$field_name}->isEmpty()) {

        $this->trackOnEntityCreation($source_entity);
        //return;
      }

It seems not to introduce problems on first quick look. But really need to understand why this return statement was introduced.

@Seb_R: I am on the same end and can confirm that removal of only one item in a multivalued field does not trigger USAGE_REGISTER event correctly.

Proposing this patch as a starting point, allowing to add a user without mail address and fixing #2 from issue description (by not showing the newsletters page if user has no mail address).

My use case is to have users without mail address for other purpose than simple news, OPs case was to have users add the mail address later. So this patch is not enough for the latter case obviously.

I suppose patch from #23 and MR60 are currently not really a working solution for 4.x (i don't know if they maybe worked for 3.x). But 4.x is badly broken with this patch, basically whole system is dead after applying this patch. It seems this patch attempts to do a lot of refactoring beside fixing the issues with user without mail and maybe this has not kept up with latest changes in Simple news.

It seems to me, starting over here would be better. And for the refactoring part, imho one should focus on fixing the issues with user with no mail address here and do the refactoring in separate issue.

When ready the issue description I am also asking myself, whether there are not some basic elements missing. It's about "when logged in as a user with blank email address". But on my end, I cannot create any user with blank email address in the first place, because this is already failing. So this would be something to solve before going to problems logging in with such a user.

So this surely "Needs work"

changing to "Needs review". New release would really be nice.

There was one more occurence of .tooltip class in textarea.css

The changes seem very reasonable, because the class .tooltip will interfere with Bootstrap: https://getbootstrap.com/docs/5.3/components/tooltips/#markup

So probably every Bootstrap theme that uses tooltip will be affected.

I can tell for Bootstrap5 → theme. The clipboard button is visually broken there.

The MR is good but the CSS class selector was missing the dot, so I changed this.

I also dare to change the category to "Bug report" as I suppose breaking Bootstrap is maybe an issue.

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

@miiimooo: Thanks for your feedback. Few questions though.

First the message you gave in the issue description is cut off. Could you maybe post full message?

I suppose this warning message does not affect every 8.3 setup. I tried with php 8.3.8 and this video https://youtu.be/hTWKbfoikeg?feature=shared - I do not experience the (any) warning message.

Can you give the url and exact setup that leads to the message on your end?

And regarding the code change for parsing the URL. I am open to change this like it is used in video_embed_field. But the change you proposed brings in more changes as far as I can tell. Could you explain your code?

That said, currently I can insert URL like
https://youtu.be/hTWKbfoikeg?feature=shared
https://www.youtube.com/watch?v=hTWKbfoikeg

They work with youtube duration.
You proposed https://www.youtube.com/w/hTWKbfoikeg - but I suppose ombed media in core does not support this type of string, so seems not to make sense to support this via youtube duration.

It's true that youtube shorts are currently not supported by youtube duration and they could and should be supported. But this could imho be solved with a lighter patch than the one you currently provided.

I suppose it should be mentioned in README that you may need to add this to .htaccess manually.

The default for Drupal is to create .htaccess with 444 permission, not writable:
https://git.drupalcode.org/project/drupal/-/blob/02507ab71932f2b574c777a...

So only in a non-default setup this might work automatically.