🇬🇧United Kingdom @Wongjn

Thank you for your offer, I will take you up on it. You have been added as a maintainer to the project, thank you!

Thank you for bringing the core issue to my attention. Yes, this would feel like a good direction for the next major version.

Thank you for your contribution! I've reviewed your work with some points to take into consideration please.

The issue comes from how are generated the options of the widget, width and height are not set. So the the condition of the line 88 in the template_preprocess_ex_icon() don't apply.

Could I check what the condition on line 88 is please? From what I can see, that line is outside the template_preprocess_ex_icon() function and is a line of the documentation block, unless I am mistaking what the line number is referring to.

I'd prefer we get to the root of the cause of the warning in template_preprocess_ex_icon() rather than rework render arrays. This is because any code that uses the ex_icon #theme could cause this warning if they don't pass width and height #attributes, so I'd like to fix it at the "origin" point, so to speak.

Hi! Thank you for your report and contribution!

Can I just double-check you are running 8.x-1.7? It seems dubious that the error message is reporting line 88 when that is outside the template_preprocess_ex_icon() function. Furthermore, we do isset() checks on lines 72–74:

if (isset($variables['attributes']['width']) xor isset($variables['attributes']['height'])) {
  $numeric_width  = isset($variables['attributes']['width']) && is_numeric($variables['attributes']['width']);
  $numeric_height = isset($variables['attributes']['height']) && is_numeric($variables['attributes']['height']);

So the error shouldn't ever come up.

Indeed, even in the PHPUnit tests for 8.x-1.7, no such error is surfaced.

With these two points, I am leaning towards something within your setup specifically. However, if you could provide anymore details to replicate the problem and/or a PHPUnit test to go alongside your MR, that would be most helpful.

Wongjn → created an issue.

Wongjn → created an issue.

Wongjn → created an issue.

Wongjn → created an issue.

Thank you for the contribution! I made some slight changes:

• Add some arbitrary length to max-height, so that there is a visual indicator of more items.
• Changed overflow-y to auto, so that a scrollbar only appears when needed.

#6: I believe the change here is wholly incorrect – I do not believe the title should be the default value of the value form element. The value form element is for the icon ID.

The original patch by Ajeet Tiwari is great, I'll work on a test for it.

Looks good to me, thanks!

Wongjn → created an issue.

Thank you for your contribution! Back to Needs work as the patch does not apply (seems to be incorrect file path).

Thank you for the contribution! It seems like this could be a breaking change in 8.x-1.x if there is any extra hook_preprocess_HOOK() that runs after, for example. Could we consider this change exclusively in 2.x instead please?

As stated previously, needs tests.

Thank you for the contribution! Could possibly have meant to set the status of this issue as Needs Review or are you still working on this? Regardless, we could use a test here I think!

Yep! The test is included in the MR.

Shouldn't it be possible to run d7_block without d7_custom_block (and thus implicitly, without the Block Content module)? The constructor for the BlockPluginId tries to consider this fact, but I believe it was a simple programming error that needed to pass FALSE as a second argument for the ternary, otherwise this ternary would serve no purpose.

Thank you for the contribution! We could use some tests please.