🇺🇸United States @lemming

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.