🇨🇦Canada @nikathone

andileco → credited nikathone → .

nikathone → changed the visibility of the branch 3457776-fix-triggers to hidden.

Added the test for views using the filter. This is ready for review.

nikathone → created an issue.

Merge the code to dev.

Marking this fix.

nikathone → created an issue.

Sorting the options ASC without adding any configuration for the user. I think should be kind of expected. @ultimike please let us know if you still think the option to let the user opt in into sorting is needed.

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

andileco → credited nikathone → .

andileco → credited nikathone → .

nikathone → created an issue.

andileco → credited nikathone → .

andileco → credited nikathone → .

nikathone → created an issue.

Ended up adding more codes that are not related to the issue but I think they are going to help us in the long run especially with the introduction of some test coverage for the main Select.php class.

Some how the MR is mergeable but it contains a commit that I didn't make. Adding a patch file if someone need it for composer patches.

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

Looks good to me.

andileco → credited nikathone → .

andileco → credited nikathone → .

andileco → credited nikathone → .

andileco → credited nikathone → .

nikathone → created an issue.

andileco → credited nikathone → .

I was having js errors when attempting to close the modal and the latest code in the MR fix the issue.

andileco → credited nikathone → .

andileco → credited nikathone → .

@sime thank you for the code and the issue. I have updated the code to move the check into a separate function just in case, in the future, we come across another way of checking if a theme is related to Olivero. Let me know if this change still work as expected for you and this issue should be ready for commit.

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

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

My work around was to implement a hook form alter and removed the undefined bundle from being picked up by the content language settings form.

<?php
/**
 * Implements hook_form_FORM_ID_alter() for language_content_settings_form.
 */
function mycustom_module_form_language_content_settings_form_alter(array &$form, Drupal\Core\Form\FormStateInterface $form_state, string $form_id) {
  // Unset undefined bundle added by the file entity to avoid a LogicException.
  // See https://www.drupal.org/i/3417971
  if (!empty($form['settings']['file']['undefined'])) {
    unset($form['settings']['file']['undefined']);
  }
}
?>

I also think a solution should be implemented in this module because it's the one that introduced the undefined bundle. Setting this to major because it's stopping people with translation enabled to update the settings for content language.

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

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

@cdeces can you give the MR a try? It should solve the issue. You might also need to re-save the view.

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

If the test pass which I think it will, this should be committed. Also @lars.stiebenz please don't change the version because that's where the commit will be applied to. Also let see if we can get another person than me and you to RTBC. Thanks for the patch.

Let see if the tests will pass. Also is there a way you can provide a sample code of how the override is done?

andileco → credited nikathone → .

nikathone → created an issue.

nikathone → created an issue.

+++ b/src/Element/Chart.php
@@ -202,12 +203,11 @@ class Chart extends RenderElement implements ContainerFactoryPluginInterface {
+      throw new PluginNotFoundException($plugin->getChartName(), 'Chart type @type not supported by @plugin', [
+        '@type' => $type_name,
+        '@plugin' => $plugin->getChartName(),
+      ]);

Maybe we should throw a logic exception here instead of the plugin not found one. This could be something like throw new \LogicException(sprintf('The provided chart type "%s" is not supported by "%s" chart plugin library.', $type_name, $plugin->getChartName()));.

If you agree with the above then we should also update the test to catch the logic exception.

+++ b/tests/src/Kernel/ChartTypeSupportTest.php
@@ -0,0 +1,58 @@
+    $this->expectException(PluginNotFoundException::class);
+    $this->assertNotEmpty($this->renderer->renderRoot($element));

I don't think we need to assert that the element is not empty since our primary goal is to check for the exception. However we need to change the expected exception to $this->expectException(\LogicException::class); and also maybe check for the expected message to see if it includes our "not_supported" type and the "charts_test_library" something like $this->expectExceptionMessage('The provided chart type "not_supported" is not supported by "Charts Test Library" chart plugin library.'); should do.

+++ b/tests/src/Kernel/ChartTypeSupportTest.php
@@ -0,0 +1,58 @@
+    $element = [
+      '#type' => 'chart',
+      '#library' => 'charts_test_library',
+      '#chart_type' => 'bar',
+    ];
+
+    // Assert that bar chart type is supported. The chart should be rendered.
+    $this->assertNotEmpty($this->renderer->renderRoot($element));
+

This part is already covered by https://git.drupalcode.org/project/charts/-/blob/5.0.x/tests/src/Kernel/.... If for example in that test they were using a not supported type the test would fail.

andileco → credited nikathone → .

Did a quick review of the code and some initial manual test. I think we need to create a follow up issue for adding test coverage or commit the current patch, then change its status to needs work and tag it with need tests.

andileco → credited nikathone → .

Can we get a new release that contains the latest update?

Which theme are you using?

andileco → credited nikathone → .

andileco → credited nikathone → .

nikathone → created an issue.

I tried to reproduce the issue following the steps in the description but I couldn't. I ended up with two basic page content with two different languages. The two content are not connected as in being the same content with two different translation.

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

nikathone → created an issue.

andileco → credited nikathone → .

andileco → credited nikathone → .

@themodularlab I think there is no need to use Xss::filter($caption['#plain_text']); because drupal should take care of that. See https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co... and https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co.... This means you can safely do $config[$key]['item_config']['caption'] = $caption; when caption is an array.

I think I think doing

$caption = $value['item_config']['caption'] ?? NULL;
$config[$key]['item_config']['caption'] = is_string($caption) ? Xss::filter($caption) : $caption;

should be enough unless caption is not supposed to be set to NULL. In this case they should some validation somewhere else in the code to stop this from happening.

We had to apply a local patch for the time being. Not sure if this is the right approach though.

It looks like this was partially fixed. I just experienced

TypeError: strlen(): Argument #1 ($string) must be of type string, array given in strlen() (line 477 of /code/web/core/lib/Drupal/Component/Utility/Unicode.php)
#0 /code/web/core/lib/Drupal/Component/Utility/Unicode.php(477): strlen(Array)
#1 /code/web/core/lib/Drupal/Component/Utility/Xss.php(65): Drupal\Component\Utility\Unicode::validateUtf8(Array)
#2 /code/web/modules/contrib/tb_megamenu/src/Entity/MegaMenuConfig.php(176): Drupal\Component\Utility\Xss::filter(Array)
#3 /code/web/modules/contrib/tb_megamenu/src/TBMegaMenuBuilder.php(149): Drupal\tb_megamenu\Entity\MegaMenuConfig->getMenuConfig()
#4 /code/web/modules/contrib/tb_megamenu/includes/tb_megamenu.theme.inc(254): Drupal\tb_megamenu\TBMegaMenuBuilder->getMenuConfig('main', '...')
#5 [internal function]: template_preprocess_tb_megamenu(Array, 'tb_megamenu', Array)
#6 /code/web/core/lib/Drupal/Core/Theme/ThemeManager.php(287): call_user_func_array('template_prepro...', Array)

Did some initial manual test and noticed that we might still need to update the text for the summary of the autocomplete widget.

Which charts library are you using?

This issue has been addressed. I will go ahead and closing this as outdated but @kopeboy please feel free to reopen it if you experience it after updating to 5.0.6.

@mhentry please let us know if the example above helped you. In the meantime I will go ahead and close this issue.

Can you provide a screenshot of your bar chart and an example of what you would like it to look like? Here is a page where there is bar chart examples for highcharts. You can click the jsfiddle or codepen link to edit the data.

@hmdnawaz here is an obvious question but I think I ask to get make sure that you did enable at least one of the charts library module? Also can you, please, let us know at what line in web/modules/contrib/charts/src/Element/BaseSettings.php you printed the $element?

Thank you in advance for providing more information about this issue.

Thank you for the patch but if this config is being added only to be used for highcharts.

I think it should go under highcharts plugin configuration instead of the base settings element. To do this I think we can add a new reversed properties under legend in default configurations and then proceed to add the form element in the build configuration form of the plugin. The schema should also need to be updated for the new configuration property.

The patch looks good but the issue is only happening for site running PHP 7.4. I will check with the main maintainer if we should update the code to support not supported php versions.

volkswagenchick → credited nikathone → .

@kevinquillen you could also do a form alter and add an #after_build callback. Then in the after build you can add your custom element. If your approach above is working as you would like then I think it should be fine. If you are interested in the #after_build callback approach there is an example at https://git.drupalcode.org/project/charts_highcharts_maps/-/blob/1.0.x/s... where the callback is added and https://git.drupalcode.org/project/charts_highcharts_maps/-/blob/1.0.x/s... for the callback implementation.

Committed.

nikathone → created an issue.

I couldn't reproduce the issue. Closing this one for now.

This initial code is to allow single axis chart type to take advantage of color selection method when grouped. I added a custom property on the chart_data element and updated the highcharts library to take advantage of this new property to set the color.

It would be good to test this first using highcharts then I can go ahead and customize other libraries. If you want to test this using composer patch here is the path link: https://git.drupalcode.org/project/charts/-/merge_requests/53/diffs.patch

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

Pushed some that I think should help with this. See the merge request or if you are interested with patch use https://git.drupalcode.org/project/charts/-/merge_requests/52/diffs.patch

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

My last commit adds `entity_id` in the legacy schema to ensure that it stays as string. In #3270630 it was update to be an integer due to performance reasons but I would like us to keep it as string in the legacy plugin for this initial update path then open a follow up issue if the people who were using V1 face the performance problem.

Created a merge request that ensure that the Search API: Current domain can be setup within a group in a view to work with other fields like in the attached screenshot.

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

Changed this back to need work. See comment above.

This is an initial upgrade path. It doesn't cover views or page manager. Also people who might have implemented the approach to add an extras column for as recommended in v1 are not taken into account.

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

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

Cleaned up the code and added the configuration form for series keys and join by. Pushed the update through merge request but if you are interested to test this using a patch you can find one at https://git.drupalcode.org/project/charts_highcharts_maps/-/merge_reques....

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

To make sure I understand, I should try slick js library version 1.6+ and <= 1.8.0 with the latest Drupal slick 2.7.0 and report back?

Thanks for the quick feedback.

I am re-opening this to make sure that the maintainer see my latest update but they should close it in case they feel like this is an isolated problem. I appreciate all the work for the maintainer(s) put into this module.

I am not sure that this error is related to this one but after doing a google and issue search I found this is closest one with what I am facing.
Mine looks like

Uncaught TypeError: Cannot read properties of null (reading 'add')
    at e.initADA (slick.min.js?v=1.x:1:19335)
    at e.init (slick.min.js?v=1.x:1:19101)
    at new <anonymous> (slick.min.js?v=1.x:1:2832)
    at i.fn.slick (slick.min.js?v=1.x:1:42781)
    at doSlick (slick.load.js?v=9.5.2:296:7)
    at nn (dblazy.min.js?rpbwad:1:2287)
    at N.once (dblazy.min.js?rpbwad:1:8063)
    at Object.attach (slick.load.js?v=9.5.2:321:12)
    at drupal.js?v=9.5.2:24:24
    at Array.forEach (<anonymous>)

It started after updating to the latest Drupal slick 2.7.0 from 2.6.0. The slick js library that I am using didn't change (v1.8.1).

After reading comment #6 I went ahead and updated the module code by applying the following change:

diff --git a/js/slick.load.js b/js/slick.load.js
index 9888cca..b20541a 100644
--- a/js/slick.load.js
+++ b/js/slick.load.js
@@ -10,7 +10,7 @@
   var _id = 'slick';
   var _unslick = 'unslick';
   var _mounted = _id + '--initialized';
-  var _element = '.' + _id;
+  var _element = '.' + _id + ':not(.' + _mounted + ')';
   var _elSlider = '.slick__slider';
   var _elArrow = '.slick__arrow';
   var _elBlazy = '.b-lazy[data-src]:not(.b-loaded)';
diff --git a/slick.libraries.yml b/slick.libraries.yml
index 3da6dbd..6b8b80e 100644
--- a/slick.libraries.yml
+++ b/slick.libraries.yml
@@ -68,7 +68,7 @@ base:
 slick.load:
   version: VERSION
   js:
-    js/slick.load.min.js: { weight: 0, minified: true }
+    js/slick.load.js: { weight: 0 }
   dependencies:
     - slick/base
 

and now the error is gone.

I am not providing a patch because I am not sure if the maintainer is onboard with this approach and I couldn't figured out how to re-generate the slick.load.min.js file.

+++ b/src/Plugin/views/exposed_form/BetterExposedFilters.php
@@ -838,10 +778,21 @@ class BetterExposedFilters extends InputRequired {
+    if (isset($form['actions']['reset'])) {

@@ -933,4 +856,229 @@ class BetterExposedFilters extends InputRequired {
+            $handler->value = $handler->options['value'];

Is there any particular reason this code $handler->value = $handler->options['value']; is implemented here? I can't seem to see the benefit of having it. I went ahead and commented it and the reset button still working fine.

Asking because its presence is causing a php warning when a custom handler doesn't have a value option.

Made an initial commit that I think should be enough.