🇩🇪Germany @stefan.korn

stefan.korn → created an issue.

There is another issue if a link facet and a checkbox facet are used together. The checkbox facet js will also target links, which makes them loose styling like #3309612: Link widget styling → claims. The facet checkbox js should not target links.

@phjou: Thanks for reviewing.

With solr it will only work on the first page of paged displays. There is no solution to this yet.

A warning message when configuring the processor should point this out..

It might also be possible to make an exact match searching work with Solr alone, see https://stackoverflow.com/questions/29103155/solr-exact-match-boost-over... - but it is also no trivial task there.

We encountered the same issue and the patch from #5 fixes it. Thanks.

I change to "Bug report". And imho this is kind of a serious bug. If Media Bulk Upload does not fully support taxonomy entities this should be mentioned somewhere.

stefan.korn → created an issue.

stefan.korn → created an issue.

I also came across issue with webform submissions, but I suppose the patch from 165/166 is going to far, probably rendering the whole checking for recursion useless.

I suppose webform is really doing here something that is to be considered recursive rendering, see

https://git.drupalcode.org/project/webform/-/blob/03b6e07416480ad802f453...

This is ending up in using #theme => 'webform_submission_data' and then having this again in $variables['elements']['#theme'].

So I think the code here is still valid for checking for recursion, but we can maybe provide a possibility to circumvent the webform issue by providing a possibility to vary the render recursion key by something else than entity type, entity id and view mode. This would allow "intentionally" recursive rendering.

With this change it would be possible to solve the webform issue by using hook_preprocess_hook like this:

/**
 * Implements hook_preprocess_HOOK().
 *
 * fix regression (no submission data shown) due to patch from:
 * Can only intentionally re-render an entity with references 20 times -
 * https://www.drupal.org/project/drupal/issues/2940605
 */
function yourmodule_preprocess_webform_submission_data(&$variables) {
  if (isset($variables['elements']['#theme']) && $variables['elements']['#theme'] === 'webform_submission_data') {
    $variables['elements']['#recursion_key_variator'] = 'wrapped';
  }
}

The name of the variator can be anything, it just distinguishes the wrapped render element from the outer one.

Providing an interdiff to #155. (the patch from 155 is made of multiple commits, so when comparing the patch files you will note a lot more changes, but this interdiff is showing the real difference by comparing the effective changes via two git branches → ).

Not changing the MR right now. Not sure if maintainers agree on this, so letting MR out for the moment.

@erwangel: I committed solution to the dev branch which should do the following:

- work with any image field on any media entity type, including thumbnail

would be great if you can test this if you find the time.

@erwangel: Thanks for your review of the module and bringing up this interesting points.

Currently the module is only targeting the source field of a media with image source type.

The thumbnail is a base field of any media entity. Testing this a bit, it would work with thumbnail too if changing the code to allow for configuring thumbnail with link to parent entity checkbox and modifying the output to support this too.

This would also allow to use it on remote video too.

And thinking some more about this, I suppose it could work for any image field in any media entity.

So if changing the approach I guess supporting any image field in any media entity would be the desired result. But this is quite a bigger rework of the code. I need to work a bit more on this.

Another point:
I am not sure what image_private media source is. This is not coming from core I suppose. Is it a module that is providing this or maybe a custom implementation on your end?
But if the approach I described above would be feasible, maybe we can get rid of the media sources check all together.

stefan.korn → created an issue.

stefan.korn → created an issue.

So I have committed a more generic approach to the dev branch, that should also handle commerce product variant correctly.

On my end it is working.

Would be great if you can test it too. @harismalik

I have added composer.json to require DKAN.

Still DKAN and specifically DKAN datastore module needs to be enabled before enabling this module.

Do you have DKAN installed and enabled? This is a prerequisite for this module, see README.

Due to some issue with adding dependencies to projects outside Drupal, i could not add the DKAN module as a dependency in the info.yml. See https://github.com/GetDKAN/dkan/discussions/4149

We could at least add a composer.json to this module which requires DKAN, but still this will not automatically install DKAN together with this module because the dependency is not included in info.yml.

Thanks for the explanation.

As mentioned in 🐛 Workspaces can't be published from the command line Fixed devel quits with WSOD on "Event info". And yes, this is happening if the content moderation module is installed and the workspaces module is not.

Basically

Drupal::service('event_dispatcher')->getListeners();

is failing then.

It seems to me that getListeners() without event argument is a valid call, therefore I am wondering if this should really end in error here (no exceptions for the method pointing in that direction either) even if the workspaces module is not installed.

And the difference between service definition (optional) and implementation (required) does also seem not really straightforward.

I have amended the MR to make the change in the implementation and not the service defintion.

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

What is the deal about having the second argument optional here:

  content_moderation.workspace_subscriber:
    class: Drupal\content_moderation\EventSubscriber\WorkspaceSubscriber
    arguments: ['@entity_type.manager', '@?workspaces.association']

but required there:

class WorkspaceSubscriber implements EventSubscriberInterface {
...
public function __construct(
    protected readonly EntityTypeManagerInterface $entityTypeManager,
    protected readonly WorkspaceAssociationInterface $workspaceAssociation
  ) {}

as pointed out in 🐛 WorkspaceSubscriber service parameter $workspaceAssociation must be optional Needs work .

I came across this when devel quits the event info with an exception.

Actually

Drupal::service('event_dispatcher')->getListeners();

does not seem to work because of the difference between service definition and class implementation.

I changed the MR to comply with #13 and take advise from #10:

Why didn't you use recordation from compiler?
To preserve current behavior: math.abs(40%)

Reverted the change in dist/bootstrap/5.2.3/scss/_alert.scss, because bootstrap itself is using this: https://github.com/twbs/bootstrap/blob/v5.2.3/scss/_alert.scss#L65 (so probably this won't get changed anymore for BS 5.2)

including patch from MR

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

I suppose this issue is now fixed by ✨ Increase max length on API URL field Fixed , closing as duplicate

There seem to be related issues. The check for "canonical" template was introduced to mitigate a problem with non linking entities (paragraphs). So there is probably an issue if just checking for method toUrl, probably any entity has this method.

The paragraph entity is throwing an exception for the canonical link template. Probably a solution would be to get away from checking the "canonical" template and just getting toUrl on the parent entity but put this in try/catch to avoid exception and just do not put out a link in case the exception is caught. That way behavior would be like before, non error on non linkable parent entities, but the toUrl would work independently whether the template is canonical or something else. That will be more flexible and would solve this issue in a generic way probably.

Thinking about this and testing, but seems like a good approach.

@harismalik. Thanks for pointing this out. Interesting problem. I tried it out too.

First of all, I suppose the steps to reproduce would be

1. Create a commerce product variation with a media image.
5. Click on a product variation image.

Because just with a product this seem to work with the canonical link.

Now looking at the product variation entity, I suppose it doesn't especially matter if the collection link template does exist. It just matters that the product variation entity does not provide a canonical link template and it would be sufficient here to just check if it is product variation entity.
So something like
if (!$parententity->isNew() && ($parententity->hasLinkTemplate('canonical') || $parententity->getEntityTypeId() === 'commerce_product_variation'))
would probably do it as well.

But it seems a bit odd to just check for the entity type, a more generic solution would be better imho.

The product variation entity provides a method toUrl which handles the creation of a "canonical" link. So basically the product variation has a canonical link, but not provided as link template.

So for a more generic solution I am thinking if
if (!$parententity->isNew() && method_exists($parententity, 'toUrl'))
could do it. It works for the case given with the product variation. But at this moment I am not exactly sure if this has some undesired implications.

On the other side, I am not sure why product variation does not provide a canonical link template (probably because it just uses query params to alter from product url) and whether maybe commerce could provide a canonical link template for product variation entity as well.

stefan.korn → created an issue.

Hm. I think recline.js like it was used in D7 DKAN is dead. Looking at https://okfnlabs.org/projects/reclinejs/ it seems this is now "portal.js" which is really different I suppose, see https://github.com/datopian/datahub/issues/520 for reasoning about the switch.

With this module I have no plans to integrate the old recline.js neither portal.js. The dkan_chart module is about integrating charts in DKANv2 with chartjs. I am currently not planning to broaden the scope to other frameworks, since it seems a big enough scope to integrate with chartjs.

That said, I am also still searching for a way to have data visualized in a table style like it was provided in D7 DKAN with recline. But from looking at portal.js at a glance, I suppose I will not turn to portal.js for this nor will I turn to old recline.js. I will search for another solution and maybe make a separate module for DKANv2 from that.

From what I understand from really quickly looking at portal.js, this could be an alternative frontend for (decoupled) DKANv2 (an alternative to the React frontend prototype that is provided now). But I have no ambition for integrating with portal.js though it looks interesting. My focus is currently on using Drupal itself for the frontend (not decoupled) and having the necessary features that my client expects for their open data platfom with DKANv2.

3.x is now requiring ckeditor5/internal.drupal.ckeditor5 as a library dependency.

Though this works as expected and fixes the issue mentioned here, it has a minor issue if ckeditor5 module is not installed.

You will get warning message "The following theme is missing from the file system: ckeditor5" during installation of maxlength.

This could be fixed by requiring ckeditor5 module in info.yml. But in a scenario where you do not want to use CKEditor5 at all, it seems not correct to force the installation of ckeditor5 module, because maxlength will in principle also work without it.

So maybe one wants to tolerate the warning message during installation. Or one could maybe use hook_library_info_alter to add the dependency just in case ckeditor5 module is active.

Okay, once more.

It seems setFullTextFields() is possibly called twice in case the "override used fields" is configured:
https://git.drupalcode.org/project/search_api_autocomplete/-/blob/8.x-1....

This is overruling:
https://git.drupalcode.org/project/search_api_autocomplete/-/blob/8.x-1....

So maybe that is intended, but seems at least not intuitive.

Inserting

      if (!$query->getFulltextFields()) {
        $query->setFulltextFields($this->configuration['fields']);
      }

here might solve this.

Oh, it seems it depends on whether "Override used fields" in "Configure Suggester Retrieve from server" is configured or not.

If there are fields configured, that it will not work as expected (showing suggestions in all exposed fields) and if there are no fields configured, it works like expected.

I suppose that this bug is still persisting. Still getting the suggestions from all fields provided in any exposed filter (if having multiple fields exposed though). Even if there is an exposed field for which no suggestions where activated, the suggestions for the other fields will be provided there too.

I could pin this down to:
https://git.drupalcode.org/project/search_api_autocomplete/-/blame/8.x-1...

if I comment this line out, it seems to work like expected. But I am not sure what's up about the $input and the $single_field_filter here.

But the setExposedInput() - https://git.drupalcode.org/project/drupal/-/blob/10.2.x/core/modules/vie... - expects a string[] array. The actual implementation for the $single_field_filter case is not in line with this, at least my IDE is complaining.

Hm, still having trouble with 2.x and path aliases (maybe I have overlooked somehting, but I can not get a clean solution with path_aliases. Tried different things, exporting path_alias, removing the 'pid' entry from node export etc.).

Based on #7 (path.alias_storage is gone now) one could use something like this:

$url_aliases = array(
  'b64cb122-d96c-467d-b22d-901d8ad3232c' => '/home',
  '90382fbc-d908-4a27-99e9-2e7017eb2853' => '/api',
  '75590ebe-b559-4080-9467-c7ff7579cba3' => '/about'
);

foreach ($url_aliases as $uuid => $url_alias) {
  $node = \Drupal::entityTypeManager()->getStorage('node')->loadByProperties([
    'uuid' => $uuid
  ]);
  $node = reset($node);

  if ($node instanceof \Drupal\node\Entity\Node && isset($url_aliases[$uuid])) {
    \Drupal::entityTypeManager()->getStorage('path_alias')->create(
      [
        'path' => '/node/' . $node->id(),
        'alias' => $url_aliases[$uuid]
      ]
    )->save();
  }

You can for example put this in a .php file and then execute this via drush scr after the default content has been imported.

Could have also use it in hook_node_insert as per #7, but seems not the cleanest way to run through this every time a node is inserted, though this is kind of a one time task.

I can confirm that this patch works.

And I can also confirm that the recent date processor can have weird effects on the results for future dates without this patch (and the option for future dates enabled).

while patch from #67 fixes the issue and enclosure tag with image can be provided, there are no such things possible for the enclosure field as rewriting results or no results behavior.

I was in need to provide a second field as a callback for the enclosure in case the first field is empty.

I have therefore amended the patch to optionally configure a second enclosure field as fallback.

I do not want to endorse this as a clean solution, so I am hiding the patch, but provide it here in case someone might be need of something similar and also to remind us, that maybe more flexibility with enclosure field would be great.

Isn't this a bit overdoing to disable inline form errors in general when the issue is only about a specific field widget?

against latest HEAD

now this patch based on #15 but applying to 2.x branch.

So testing this, it seems without Markup::create special characters in the title will indeed be double escaped. This would mean #15 is still the way to go.

@Anybody: Okay, I am not sure about the double escaping. So we would need the Markup::create still to resolve this?

And changing to 2.x branch as this is current.

I am a bit confused about the status of this issue.

Imho patch from #14 was the best solution so far, maintainer just requesting to not use Markup::create.

Merge Request #40 seems a bit of a mess now, i. e. what is "commerce_product_ui_product_form" doing in there?

I am providing patch based on #14 without Markup::create to maybe get this going again.

Regarding tests: Not sure what should be tested and how and if additional test is really necessary.

stefan.korn → created an issue.

Here is a patch based on #2.

It would be better if the source that leads to shy.js would be provided, but until then one can just patch the shy.js directly I suppose.

confirm the issue and the fix

Question is whether FILTER_UNSAFE_RAW is a replacement for FILTER_SANITIZE_STRING. Imho it is not. Reading https://www.php.net/manual/de/filter.filters.sanitize.php the recommendation is to use htmlspecialchars() and FILTER_SANITIZE_FULL_SPECIAL_CHARS is defined as "Equivalent to calling htmlspecialchars() with ENT_QUOTES set". So FILTER_SANITIZE_FULL_SPECIAL_CHARS might be a better replacement as FILTER_UNSAFE_RAW, but it is still not the same as FILTER_SANITIZE_STRING.

So assuming that FILTER_SANITIZE_STRING was used for a purpose, FILTER_UNSAFE_RAW is probably not fulfilling that purpose.

I think it is a dependency issue which is rather hard to pin down. ckeditor5.js needs to be called before maxlength.js. This is not assured because maxlength does not set a dependency on this. Maybe if some other module is requiring maxlength, this may result in a change of the dependency order and maxlength.js comes before ckeditor5.js.

We need to find a solution which doesn't disrupt the user experience and doesn't require any action from the users.

true surely, but not so easy as far as I can see.

For me now it is better to decide when to load the ckeditor5 library myself, rather than loading it just on any occasion which the previous patch does.

How about this patch, which introduces a new setting for CKEditor5 library dependency.

Maybe could use allowed settings for this too and only allow on formatted text fields.

One thing I can think of, would be to introduce a new setting for maxlength like "require CKEditor5" and include the dependency only based on this setting.

I can also confirm this issue in some setup. I believe it is not especially tied to CKeditor5 in paragraphs, because in my setup though we are using paragraphs the CKEditor5 field with max length setting is not used in a paragraph.

I suppose it can happen that the maxlength.js is called before the ckeditor5.js and this is causing the issue, because the ckeditor5Id is not availabe for maxlength then: https://git.drupalcode.org/project/maxlength/-/blob/79379c927380d1796f38...

Adding the dependency on ckeditor5/internal.drupal.ckeditor5 solves the issue by assuring that ckeditor5.js is always called before maxlength.js.
This seems to be an easy solution for this problem, though it probaby is not nice for supporting CKEditor4 and would also request that library for non CKEditor fields with maxlength. A clean solution would need to distinguish whether we are on CKEditor5 field before attaching the library and then maybe have a library for ckeditor5 especially that has the dependency on ckeditor5/internal.drupal.ckeditor5. But this does not seem easy (the distinguishing part).

stefan.korn → created an issue.

Hm, at least for carousel this does not work as expected I suppose:

https://git.drupalcode.org/project/varbase_bootstrap_paragraphs/-/blob/9...

loop.first is not reached, therefore no item is active initially and the carousel does not show.

I suppose patch from #4 should have been used as the base for the change, but instead patch from #3 was used.

The bottom line is that there was conditional filtering in the for loop which has been deprecated with Twig 3. Patch from #3 removed the conditional filtering and moved the if condition in the for loop, but this breaks the bootstrap because loop.first is not correct then. Instead with patch from #4 the conditional filtering is Twig 3 (and Twig 2.10) compatible as it uses twig filter.

So the current state of the module is:

9.0 is back to old conditional filtering with if in the for loop, which is only Twig 2 compatible

9.1 and 10 are not using conditional filtering anymore (putting the if inside the for loop) and thus breaking the functionality. So for 9.1 and 10 this should be changed to use the TWIG filter instead as proposed with patch from #4.

I suppose if you choose "Nothing" for "Link image to" you will receive no link in the case you describe.

The link to the media edit page is the default from Drupal core I suppose and media parent entity link will only come into play if there is a parent entity and this parent entity has a canonical link.

So I think if you choose "Nothing" for "Link image to" and still check the box "Link to parent entity (if appicable)" you may get the same output that would come with your patch.

tending to close this with "works as designed", if you do not have any more comments on this.

@FineX: what have you chosen for "Link image to" in display configuration for this media type?

I mean this is not straight forward:

You have save a menu link with different title than the node title.
You come back later and remove the menu title and save the node.
You still have the menu link, but now with the node title again.

This has been a bug so long, it has gotten a feature ... ;-)

In a project we rely on the old behaviour to realize a special workflow regarding creation of menu links while node editing. Now with the new behavior we have a bc break going from D9 to D10. Yes, I know this can happen going from D9 to D10.

I am also wondering if there might not be people that just clear the title to remove a menu link and then wonder why they get the menu link back even with different text if they changed the menu title before. And additionaly if you for example edit nodes from the content overview you will not get directed to the node view and might even not realize that this has happened.

I am not clearly seeing the bug in the old behavior and it was there for a long time ... But surely I have been 10 years to late for lamenting on this, but I really thought this behavior was intended the way it was.

reroll for D10

rerolled for 3.x

not sure if there should be some modifications due to dropzonejs integration now being a submodule of media_bulk_upload. It works as before if the dropzone integration is enabled. I have not tested without dropzone integration, whether there are problems and whether the minimal dimension validation is provided by the managed file element by itself.

stefan.korn → created an issue.

how about this patch

stefan.korn → created an issue.

Ah yes okay understand how: .patch file splits in different commits. Sorry for the noise.

Still the MR workflow is not my favorite.

This merge request thing is kind of nasty ...

There is a diff in code between
https://git.drupalcode.org/project/webform_views/-/merge_requests/11.diff
and
https://git.drupalcode.org/project/webform_views/-/merge_requests/11.patch

at least on my end (maybe cache somewhere inbetween?).

So the patch I provided did not contain the latest code of the merge request. Therefore providing new one based on .diff from merge request.

Off-Topic: Asking myself if that merge request thing as really that good, does at least seem not that stable from a point of using the patches, maybe it's easier to handle for maintainer, new committers that maybe want to do it in web ide. But if you're used to old way, it does not seem easier and has this risk of getting unreliable patches ... hmm, feels like I will stick to the old way for my own projects.

Uploading the patch file from (current state) of merge request here, if someone likes the old way of providing patches and because directly using merge request diff as composer patch bears some risks (see https://drupal.stackexchange.com/questions/298567/patch-drupal-org-proje...).

I was wondering why this part

        $this->query->getTableInfo($this->tableAlias)['join']->extra[] = [
          'field' => 'property',
          'value' => '',
        ];

was removed for the non-multiple case. Shouldn't non-multiple case be handled just as before?

That said: without this code part I had issues with duplicate rows in views of webform submissions where I did not even use a multi value field.

So I have added this again to the merge request.

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

For a single value field this is not a Gin issue, as Claro is looking the same:

And the separator label is not aligned really out of place, just more to the bottom as with multi value field. So one can maybe accept this discrepancy. But surely consistent look between single value and multi value would be preferable. But as this is not direclty Gin related, it probably should get fixed within the smart date module. Feel free to open an issue there or I will do if I find the time.

For Gin I suppose this patch is complete and fixes the issue for multi value fields as described.

@djsagar: This is a single value field? Probably there is some other css selector combination causing the issue with single value field. Need to have a look at this.

Improved patch to handle unsigned integer fields

stefan.korn → created an issue.

@Martin: I opened an issue with Gin 🐛 bottom margin in multi value field table display Needs review that relates to the separator string in smart date, but is a more general gin issue imho. Just wanted lo let you know, so you maybe can have a say in the Gin issue as well or just for your information.

Nothing new to this issue though.

stefan.korn → created an issue.

Yes, the unfiltered view still shows duplicates. I did not realize this yesterday, but with the full patch I get a lot of more duplicates.

Seems to me that this is not the right solution either full patch or half patch. It is probably hard to solve because of the way that webform stores multiple values.

If I am using patch from #13 I get a lot of duplicate result rows and also cannot get rid of them neither by filter setting for reducing duplicates nor by view database distinct setting.

But if I only use first part of the patch, it works nice. No duplicate result rows and can filter after each delta value.

I am absolutely not sure what is happening in the first and second part, so probably I am misunderstanding something here. But patch from #13 does not help in my case.