🇺🇸United States @lemming

Only including the pipelines for 2.0.x branch, as I'm planning to discontinue work on the 1.0.x version.

Tested the pipelines and made the needed updates. As for my spelling, and typo rate, I'll work on that, but have a scan point them out helps. Thanks!

Looks good, I didn't realize I had to watch my spelling and grammar as much here.

Thanks for the patch.

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

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

I have a few warnings and spelling mistakes to address, but the CI / CD appears to be working well.

Thank you!

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

Thanks for suggesting this.

I need read the documentation on the settings that they have provided for the CI/CD process since they moved away from providing the DrupalCI testing processes.

Do you happen to know what the triggers are for testing with this CI/CD configuration? Previously there was the ability to retest based on a schedule (i.e. Weekly) and set the DB & Drupal versions to run the tests with.

Does this only trigger on code pushes and deploys? New version of Drupal core releases?

ViewBox or height & width are required in order for browsers to fit the image to the HTML elements or space.

The BoxIcons now provides this information and works correctly with the module. I'm going to call this fixed since it has been updated on the icon provider side.

Use version 3.x for Drupal 11 and newer.

Drupal 11.1 also introduces Icon packages and UI Icon module may also end up being a recommended replacement in the future.

Fixed with the updates and release of Beta15.

The issues reported on the *.info.yml files are not there in the project source:

https://git.drupalcode.org/project/toolshed/-/blob/2.0.x/toolshed.info.yml
https://git.drupalcode.org/project/toolshed/-/blob/2.0.x/modules/toolshe...
https://git.drupalcode.org/project/toolshed/-/blob/2.0.x/modules/toolshe...

These are only being flagged because you are probably running PHPCS on the release version where Drupal has applied the version and timestamp information.

The CSS files being tagged are in fact minified and transpiled from their source files.

https://git.drupalcode.org/project/toolshed/-/blob/2.0.x/assets/widgets/... and the CSS files are intended to be packaged that way.

I've added styles for applying the default autocomplete styling starting with release Beta14

The update was made to this CSS file and is based on Claro from Drupal 10.3 & Drupal 11.
Autocomplete.css

Previously Drupal had this styling for the autocomplete element separate from the autocomplete functionality they provided. The change from Drupal core, moves the styling with the autocomplete library, which include the JQuery UI and all the Javascript files, which make sense for Drupal core, but is applying too many dependencies that the toolshed autocomplete doesn't need.

The default styling is very simple and provides a default. Themes, like Claro, are able to override the styling. I've also updated the JS to be compatible with Claro styling (this commit).

Thanks for reporting this issue and helping to find a resolution.

Starting with release Beta13 Drupal 11 compatibility has been supported. Additional fixes were applied in Beta14 with more stringent typehinting and static analysis checks.

Will work on PHPUnit testing.

Thank you for the kind words and the work to get a consistent icon management system for Drupal Core.

I apologize for the extended time it took to respond. I have been working on other focuses, and needed a break from Drupal. I had a few plans for some feature improvements that I'll focus on targeting with the Drupal 11.1 release.

One big one was the ability to override icons in themes. So a module could define an edit icon for use with functionality and provide a default icon, but themes could change the icon display to fit the theme aesthetic.

I'm a bit behind maintaining and building some modules, so it might take me a little bit of time to get caught up, but this is on my list.

Thanks again!

Merging this as this patch does work.

I would recommend updating to version 2.x, however, as there are other bug fixes and relevant depreciation fixes on that branch.

@langelhc is there a reason you aren't able to use the 2.x version of the module?

This includes other bug fixes, but does move the minimum Drupal version 10.2 because it implements the the new exception handler.

I ask because D9 is no longer supported and Drupal 10.1 is going to lose support soon. If you're still using an earlier version of Drupal and not able to update, I'd like to look at back-porting and testing other bug fixes on 2.x as well.

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

Updates for Drupal 11 have been applied and a compatible release has been created.

Module has been updated for D11 support and is being tested.

This issue can be closed.

Created the 2.0.x branch for compatibility with Drupal versions 10.2 and newer(^10.2 || ^11).

The 1.0.x branch will soon be depreciated, and kept for legacy compatibility.

Thanks @JasonSafro this looks like a welcome change that we'd want to add.

I think the only update I'd ask for is to make sure that when the custom text option is used, that we filter the text with the "plain_text" filter to make sure there are no XSS issues with user entered text. Otherwise changes look good.

Depending on the CSS changes, if they are things that a theme should be responsible for, we shouldn't worry about that with the module.

Hi @shaal it's good to hear from you, I hope things are going well.

I think that this is a good idea, and I am happy to help get this ready. I have a couple items that I'm hoping to get your opinion on related to this.

1. The module supports multiple types of icon types (icon handler plugins). For all the image based ones this is obvious, but for SVG sprite / symbols or for font based icons this should be ignored, or should these be "deferred"? We'll need to pass this option to the icon handler and have it decide how to apply the setting.
   
   An existing example is how the "decorative" flag is being handled. The value is passed to the handler with the "IconHandlerInterface::build()" method and it builds the icon renderable array.
2. I think the option can be applied at the iconset level and the field formatter (like with Drupal Core image). So I'm thinking of having an optional "loading" option (with a global default) that has:
   • lazy - Icons are loaded lazy if the handler supports it - no option appears on formatter
   • eager - Icons are loaded with eager attribute if appropriate - no option appears on formatter
   • field - Setting is available per field - option appears on formatter with a preferred default pre-set

I'm interested in working on this more in the next week or so, if you don't beat me to it, but your input or ideas on this would be great. Especially on how you think this applies best to the other cases like font based icons (ignore this option?) and SVG Symbols & Sprites.

Rolled and updated patch that addresses the issues I mentioned previously.

I tested with BEF version 6, and tried the variants of the links, select and radio/checkbox types.

@sea2709 thanks for submitting a patch, this does look like a good update and I will include this with a couple updates.

1. Remove the "use" declaration for "FilterPluginBase" as it is no longer used and is called out by Coder / Codesniffer
2. Rather than update the EnittyBundleFilter::operators() method you can just remove it and use the parent InOperator::operators()
3. Same as above with the EntityBundleFIlter::OperatorOptions(), keep the parent version, it makes use of the "$which" parameter which you added by did not implement.
4. Because the operators being changed to all lowercase "in" and "not in", the default value in "::defineOptions()" needs to be updated.
5. Because operators are being changes to lowercase, we need to make sure the change is compatible to existing views filters configurations which were created previously. I recommend adding an override to "EntityBundleFilter::init()" method and ensure that the:
   

   public function init(ViewExecutable $view, DisplayPluginBase $display, array &$options = NULL) {
     parent::init($view, $display, $options);
   
     $this->operator = strtolower($this->options["operator"]);
   }
   

   OR

   public function init(ViewExecutable $view, DisplayPluginBase $display, array &$options = NULL) {
     $this->options['operator'] = strtolower($filter->options["operator"]);
     parent::init($view, $display, $options);
   }
   

Thanks @scottalan this does look like an error when using associations with entity types that do not have a canonical page route.

This fix does fix the issue, and I appreciate the help with getting this fix.

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

Hi @esolano thanks for finding and patching this issue.

Took me a little bit to remember why this looked so familiar. This was a changed to attempt to appease drupal-check / PHPStan analysis tools.

50 Access to an undefined property
Drupal\Core\Field\FieldItemListInterface::$alias.
💡 Learn more:
https://phpstan.org/blog/solving-phpstan-access-to-undefined-property

Which in this case is harmless but I wanted to avoid ignoring or having too many of those pile up (makes it harder to wade through actual problems). I was hoping that maybe D10 w/ PHP8 would start using the which would resolve this, but I don't think they are making that change.

#[\AllowDynamicProperties]

Any of the fixes that make drupal-check happy really complicate the code and hurts readability, so we should tell the analysis tool to ignore it for now. Could you adjust your patch to add the PHPStan ignore before both of those lines:

/* @phpstan-ignore-next-line */

Module has been updated for Drupal 10 compatibility. These suggestions have been applied.

Module has been updated with fixes for Drupal 10 compatibility.

I'm also going to push a couple fixes to the StrategyDefinition in Toolshed, to implement the `__isset()` method which is a good call out from you.

Thanks for the PR.

I'll also write some tests soon to validate the custom builders are being called (and other properties) for future fixes.

Thanks, this was a change from 1.x-2.x where the definition types changed from being an array to a definition Object.

I did notice this for some other properties, but looks like I missed this one. For the short term I'm going to implement this fix, but long term I'm going to look at implementing a IconsetDefinition class with the iconset properties clearly defined.

Will provide the short term update this in the next day or so.

Thanks

Thanks for the quick response and the updated PR.

I've merged this in, and have a release coming soon that will include this.

Hello @superman369 thanks for the bug report and the patch.

It would actually be better to avoid casting values, and instead update the conditional to check if the value is a string or not a string. Casting to a string has some hidden overhead here, and we can definitely avoid it.

I'd be okay with:

if ((is_int($key) || $key === '' || $key[0] !== '#') && is_array($child))

Or

if ((!is_string($key) || $key === '' || $key[0] !== '#') && is_array($child))

The latter might be better because it verifies that they key is associative (a string) before treating it as a string. The first option should also work because PHP only allows integers and strings as array keys.

Hi @earthday47 very sorry for letting this slip for so long. Meant to get back to this but needed to get a few other projects done first.

I've waited so long that BoxIcons has released a new version that has "viewbox" added: https://www.npmjs.com/package/boxicons

Though I still think the issue you've found here is still relevant. With SVG's the "viewbox" is important as it helps the SVG with scaling and ratio when the HTML dimensions are factored in. Distortion of the icon occurs when a "viewbox" is missing.

I'm thinking of we probably want to implement 2 additions for these changes:

• Use the width and height dimensions to create a "viewbox" (requires assumption on offset)
• Allow the iconset to provide default values. So you could add a "viewbox" value in the iconset definition, to be used when icons don't provide a value. Works for situations where this info is consistent for the iconset.