🇩🇪Germany @stefan.korn

stefan.korn → created an issue.

I am also experiencing this issue if the setting for "Allow users to change the default charting library" is unset.

It seems to me just changing the form element type from "value" to "hidden" in this case solves this issue (see MR).

But I a wondering whether this issue is newly arising, the underlying code for the setting does not have seem to have been changed recently and seems to be in place for quite a while.

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

reroll with latest head

reroll patch with latest head

The list that admin_toolbar_tools generate on admin/index is really cluttering the UI and the link titles are often confusing.

If you want to stop the output of admin_toolbar_tools before this actually is put on the site, you could use a Route Subscriber and override the core controller to exclude the links from admin_toolbar_tools.

Like so:

your_module.services.yml

services:
  your_module.route_subscriber:
    class: Drupal\your_module\Routing\RouteSubscriber
    tags:
      - { name: event_subscriber }

src/Routing/RouteSubscriber.php

<?php

namespace Drupal\your_module\Routing;

use Drupal\Core\Routing\RouteSubscriberBase;
use Symfony\Component\Routing\RouteCollection;

class RouteSubscriber extends RouteSubscriberBase {

  protected function alterRoutes(RouteCollection $collection) {
    if ($route_index = $collection->get('system.admin_index')) {
      $route_index->setDefault('_controller', '\Drupal\your_module\Controller\AdminController::index');
    }
  }

}

src/Controller/AdminController

<?php

namespace Drupal\your_module\Controller;

/*
 * hide the admin toolbar extra tools entries on admin/index as it clutters the UI.
 */
class AdminController extends \Drupal\system\Controller\AdminController {

  public function index() {
    $output = parent::index();
    if (isset($output['#menu_items']['Admin Toolbar Extra Tools'])) {
      unset($output['#menu_items']['Admin Toolbar Extra Tools']);
    }
    return $output;
  }
}

I am a bit confused about this issue.

Firstly I suppose the solution via hook_file_insert given by TO is not considered to be a patch for this module, but rather a separate solution that you could use in your own module. In that case you should not install exif_orientation module.

The solution is depending on imagick → module and does not work with ImageMagick → module.

If you use ImageMagick module and Image Effects → module the solution could be changed to this:

/**
 * Implements hook_file_insert().
 * Rotates an image to its EXIF Orientation.
 * ONLY WORKS WITH IMAGEMAGICK TOOLKIT AND IMAGE_EFFECTS module!
 */
function HOOK_file_insert(EntityInterface $file) {
  if ($file->getMimeType() == 'image/jpeg') {
    $image = \Drupal::service('image.factory')->get($file->getFileUri());
    if ($image->getToolkit()->apply("auto_orient")) {
      $image->save();
    }
  }
}

Btw:
I am not sure what's the deal about stripping the EXIF information here. I suppose this is not related to the solution given by TO, but refering to a general issue with exif_orientation module and maybe having multiple rotations. And I agree with tfranz that stripping exif information can be a legal issue and an issue in general. This is kind of data loss, since the exif information is stripped from the image itself forever. This is no solution in my point of view, at most a very bad workaround. An advantage of ImageMagick is that it keeps exif information on image styles, which GD does not. Stripping the exif information would render this advantage useless.

had some issues with the __destruct variant in a scenario where we use some custom code to start a migration via backend.

So providing an alternative solution without __destruct but inheriting nextSource() function to unlink the temp file there.

I do not say that this solutions is better than the other one, just in our custom setup it works better.

So leaving decision up to maintainers how to fix this.

One has to say, that this only happens when drupal's temporary directory is different from systems temporary directory (/tmp in most cases).

That is probably a reason why not all users are seeing this problem. But nonetheless this is a serious bug. If you heavily use the XML Plugin it can fill up the disk space.

Thanks, for checking.

I am closing this issue and will schedule a new release shortly.

Okay, I am closing this issue. If for some reason, you find yourself in absolute need to have separate permission, you may reopen.

Okay, I will be thinking about this a little more.

Maybe it would be best to allow to provide one's own sorting function via extending in your own module. I will need to implement a hook or plugin solution for this.

Another approach outside of DKAN RSS would maybe to control this via dataset schema yourself. Maybe one could introduce a property to the dataset schema, that holds the date as a "computed" value that uses for example issued as first option, modified as second and entity->updated as last resort.

Yes, this would add flexibility.

But on the other hand, is there really need to deny access to the RSS feed? Seems kind of an edge case, that one could maybe handle with custom code. If introducing an additional permission, this permission needs to be set initially. Adds a little config debt imho.

But if it is important for you and you see a use case, than I am not reluctant to add the permission.

Interesting thought: If maybe one have a closed portal that allows only some roles to access datasets (which needs contrib module or custom code though, like Node View Permissions), will DKAN RSS respect that? Probably yes, because at the end we are relying on retrieveAll() from DKAN metastore.

made the block category more descriptive, but Block should basically show up in the Block admin UI.

warning should be gone and date sorting fixed like mentioned above, also a small fix for the "desc" sort.

Thanks for pointing this out.

Since the pubDate is defined by a property of the dataset (issued by default) and this property is not necessarily required, it is possible that pubDate is empty. pubDate is also not a required property of an RSS feed. So there should be handling there in case pubDate is empty.

Question is how to handle an empty pubDate while sorting by date. The php DateTime is using "now" as default. So we could go with that as a default as well. But that seems not suitable, because the sort would change dynamically in time.

So I think, putting a dataset with empty pubDate at the end of the feed, is probably the best option.

That said, you can also arrange a different sort (or no sort) if you change the schema json. Removing the "_sort" part altogether would apply no sorting. In that case the sorting is probaby introduced by DKANs retrieveAll().

Or you can change the sort to another property of the feed, like "title", if you put the "_sort" part like this in the schema json:

  "_sort": {
    "title": "baseSortAsc"
  },

This would sort the datasets by title from A to Z.

Thanks for reviewing the module.

Did you use the dev-version of the module? The block is still in dev branch only. And the block is shown under Category "Custom". I think this can be changed to "DKAN RSS" or something.

Will provide a new version soon, together with changes from your other issues.

stefan.korn → created an issue.

Oh yes, it is resulting from faulty config. We only disabled the server via config (as config override), but not the indexes.

Hm, MR8 is not compatible with D11? Going to admin/config/system/switch_page_theme will result in WSOD. Should we hide that branch?

MR7 is compatible with D11, but the domain functionality → is silently removed from the module? At least it should be explained why this is done.

stefan.korn → created an issue.

patch from #2 not working with current HEAD.

providing updated patch

Thanks for pointing this out.

Pushed a commit that just trims double quotes from the search string before comparing for exact match.

Seems feasible to fix the problem.

Currently thinking if there are other syntax related characters that needs to be removed before comparing (Solr?). But probably the exact search with double quotes is most common.

Thanks for pointing to this module. Didn't know this module up to now, and probaby didn't have found it before I was creating this module ...

It's true, both modules seem to be very similar.

From quick look, I suppose required_menu_link has the features of "soft require" and "disabling the automatic title for the menu link" which menu_force doesn't have, while menu_force provides the option to "lock parent item" and supports "taxonomy" which required_menu_link does not.

So there are some differences, but I acknowledge the similarity.

Will check differences a little deeper and add to module description when I find the time.

The partial import still does not respect translations.

It is discussed here too: https://github.com/drush-ops/drush/issues/2069

--partial uses a very low level API and apparently bypasses a lot of the config event system ...

This is probably the reason why translations are not imported with partial.

As far as I can see, there is also no way to import a single translation config yml (yml from configdir/language/langcode/) within the backend at admin/config/development/configuration, which also points in the direction that "partial" import will not work with translations.

There is also given a workaround in the discussion mentioned above:

config-export into a temp dir
copy the contents of the --partial dir into the temp dir
config-import from the temp dir

@drunkenmonkey: Sorry, missed the hint about test and review.

I can confirm that it works for me as expected now with the patch, though I am probably only really testing the suffix/prefix part.

Regarding the other variables, I do not have a case to test on.

In principle, I think the "not empty" check is preventing ambiguousness and should be favored. Only with the result count I am in doubt if a value of "0" should be considered empty or is expected to be empty? But is it possible at all to have a result count "0" and appear in the autocomplete suggetions at all?

I suppose this addresses ✨ Allow configuring of manual checking per field Postponed: needs info as well (using a site wide setting instead of form widget setting though). Maybe maintainers should decide whether to close ✨ Allow configuring of manual checking per field Postponed: needs info now as a duplicate or not.

Ironically this interferes with 💬 Views: Separate row per webform field Needs review . I had my share in both issues ... seems you can not have everything ...

This does not play nicely together with 🐛 Webform submission view filter for multivalues Needs work .

providing MR as a proposal. If this is acceptable, than one would probably do the same for hoo_reverse_geocode_entity_field_coordinates too.

stefan.korn → created an issue.

Hi @danflanagan8, yes #states is really nice, but I always forget the syntax and need to regoogle it ;-)

We use file download for a image gallery, where we provide an option for users to download the image in full size. We use an image media for the images and editors can give a name different from the filename to the image media and we want to have the downloaded image named like the image media is named (if this differs from real filename). That works fine on almost any browser as far as I can tell and if it would not work for some browser it would take the real filename, which is also nice because it does not break the functionality.

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.