🇨🇦Canada @nikathone

nikathone → created an issue.

nikathone → created an issue.

I would say let focus on the reported error like Missing cache backend declaration for performance. and \Drupal calls should be avoided in classes, use dependency injection instead if they are not breaking backward compatibility.

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

Not sure if I am missing something but the fork doesn't contain new codes. I also switched the version to latest chart version and I think this means that the fork might need to be rebased.

I looked at the code and for me this feels like a limited use case. There is a way this can be accomplished from a custom module:

• Implement a hook_library_info_alter and swap the charts/modules/charts_highchartsjs/charts_highcharts.js with a custom js file
• Or add a custom js file that override the chart configurations using javascript. Please see the example of the library declaration at https://git.drupalcode.org/project/charts/-/blob/5.1.x/modules/charts_ap... and the javascript file overriding the config before highcharts load them can be found at https://git.drupalcode.org/project/charts/-/blob/5.1.x/modules/charts_ap.... Note the javascript weight in the library file.

Please let us know if this approach doesn't work and we can try to see if the charts module can be updated to include more method to alter the config file.

@jrochate can you open two separate feature request issues for point 1 and 2?

@rex.barkdoll can you try to update to the latest charts version and let us know if you still experiencing this issue?

nikathone → created an issue.

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

Added related issues that will help with the code added here.

We do have $chart['#view']. The users can get the view ID from it. We might also need to document this at https://git.drupalcode.org/project/charts/-/blob/5.1.x/charts.api.php?re....

Looks good for me.

nikathone → created an issue.

nikathone → created an issue.

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 → .