🇧🇪Belgium @andreasderijcke

Antwerpen / Gent
Account created on 11 July 2015, almost 9 years ago
#

Merge Requests

More

Recent comments

🇧🇪Belgium andreasderijcke Antwerpen / Gent

Patch still works for D10.2.6 on php 8.1.
The notice has turned into a warning thought.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

Patch from #8 still works on 2.0.7 and fixes the issue for us.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

@Sourav_Paul Some feedback:

  • I'm not sure how, but you have seemed to have overwritten the first commit on the merge request with the second. Following the git commands here on on the MR itself, it should be no problem to add commits to an existing MR. Something to double check in the future.
  • About the update hook, 2 aspects:
    • The convention is to keep update hooks in the .install file, not .module and follow the existing update hook numbering unless to indicate major version jumps, which is not the case here.
      So the last one was 10001, so this one becomes 10002 and not 11001.
    • Update hook code itself was good, just didn't take possible language override into account.

In anyway, thanks for helping out.

@nightlife2008, thanks for reporting and patch.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

The 3.0.x branch has been unflagged as 'supported', I guess.
The alpha releases are still there: https://www.drupal.org/project/mailjet/releases/3.0.0-alpha2 .

If would be helpful if someone fixes this, so it is clear the module is not abandoned (since there is recente activity on the dev branch).

🇧🇪Belgium andreasderijcke Antwerpen / Gent

@Sourav_Paul Are you able to add update hook too, for potential installs that already have config on the wrong config key?

🇧🇪Belgium andreasderijcke Antwerpen / Gent

If anyone can add the update hook, this can be released faster. I might not get to it before wednesday.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

The merge request is agains the wrong branch. I can't fix it, probably only the author and maintainers can. Can't find an option to start new MR from the same fork either.

Setting the 2.x branch as default might prevent mistakes like this.

@jonathan1055, @smustgrave Can anyone of you look into this?

🇧🇪Belgium andreasderijcke Antwerpen / Gent

With "the attributes for CookiePro", I'm not entirely sure what you need, but I guess the info is on this page: https://my.onetrust.com/s/article/UUID-71e7d0a8-03d8-e272-e683-72cadded2ecf

If you need the exact consent categories (like 'C0004'), as those can be changed in CookiePro, they need to be configured in the module as well.
\Drupal\cookiepro_plus\CookiePro::getCategoryIds() will return them, keyed by the internal key constants as defined in https://git.drupalcode.org/project/cookiepro_plus/-/blob/1.0.x/src/Cooki... (CONF_CATEGORY_...).

🇧🇪Belgium andreasderijcke Antwerpen / Gent

Set to review to get feedback on the current state.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

andreasderijcke made their first commit to this issue’s fork.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

andreasderijcke changed the visibility of the branch 3388462-references-to-cookiepro to active.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

andreasderijcke changed the visibility of the branch 3388462-references-to-cookiepro to hidden.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

andreasderijcke changed the visibility of the branch 3388462-references-to-cookiepro to hidden.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

Based on the latest info I'm reading on OneTrust community pages and other

🇧🇪Belgium andreasderijcke Antwerpen / Gent

@ClassicCut Can you test/confirm the change of the MR please?

🇧🇪Belgium andreasderijcke Antwerpen / Gent

That's indeed interesting. Could be a core bug.

In anyway, to make sure that, from the module point of view, the LanguageRequestSubscriber has run first, the decorator approach might work better. I can't find an example in core of decorators on event subscribers, but it seems to work.

The decoration_on_invalid isn't supported by the core YamlFileLoader, but it works when registering using a service provider.

I do need to extract and cleanup my code changes a bit and then will post a patch for you to test.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

I guess the idea was to not have a hard dependency on the language manager service?

I think the reason is, that if LanguageRequestSubscriber runs, it must run before our subscriber, so

  1. we are sure the language override is properly set,
  2. we want to update our config ASAP afterwards, before other stuffs starts using our service

What I still don't get, is why it runs after the LanguageRequestSubscriber in your case. Is the weight of that event subscriber changed?
A quick install of the WxT 5.1.0 codebase indicates it should be the same as default core (255), while mine is 250:

How does that look on your end?

In the mean time, I'll see if there is a way to make sure ours runs always after LanguageRequestSubscriber.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

Actually, the services is registered conditionally: https://git.drupalcode.org/project/cookiepro_plus/-/blob/1.0.x/src/Cooki...

I don't recall why we made it dependent on the presence of the 'language_request_subscriber', but it seems unnecessary right now.

The 'language_request_subscriber' service is also registered conditionally, see https://git.drupalcode.org/project/drupal/-/blob/10.2.x/core/modules/lan.... Perhaps the isMultilingual() does not return TRUE in your case?

I'm curious to know if this is the root cause of your observations, and if so, if that it is (still) relevant or not.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

Ah, good catch. Didn't even notice that.
Weird that it can work sometimes without... What core version are you using?

🇧🇪Belgium andreasderijcke Antwerpen / Gent

Alright, I'll keep an eye out for possible ticket. If there is something we can do to prevent collision(s) with other modules...

🇧🇪Belgium andreasderijcke Antwerpen / Gent

@ClassicCut Can you please create a new support issue for your case, and explain it a bit more into detail + some specs? Perhaps also some screenshots of your language and language negotiation setup too.
The config override is set in event subscriber https://git.drupalcode.org/project/cookiepro_plus/-/blob/1.0.x/src/Event...

🇧🇪Belgium andreasderijcke Antwerpen / Gent

Thanks, small fix, lot less warnings :)

🇧🇪Belgium andreasderijcke Antwerpen / Gent

andreasderijcke changed the visibility of the branch 3436510-add-gitlab-ci to active.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

andreasderijcke changed the visibility of the branch 3436510-add-gitlab-ci to hidden.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

andreasderijcke changed the visibility of the branch 3436510-add-gitlab-ci to hidden.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

andreasderijcke created an issue.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

andreasderijcke created an issue.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

@nicxvan: Warnings are gone.
The context was conditional fields on block content entities.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

@Dubs We kind of have this already, but not configurable through a UI. See https://git.drupalcode.org/project/cookiepro_plus/-/blob/1.0.x/examples/... and https://git.drupalcode.org/project/cookiepro_plus/-/blob/1.0.x/examples/....

When reviewing and comparing with your current approach, a few concerns came to mind:

  • Specificity: Categories should be assigned at least at library level, not module level, so a module can support progressive enhancement of functionality depending on the given consent.
  • Aggregation incompatibility: this might be a problem in my solution as well, I've never used in the field so far, and keeping as much scripts aggregated as possible is a must have.

I need to wrap my head around this a bit more in the next weeks, to see if this might be worth investing time on this addition, without having a known demand.
I've always felt the 'auto-blocking' magic a bit too obscure and so far discouraged the use of it in combination with Drupal, in favour of complete control of what is to be blocked and when.
Anyway, to be continued.

If you have additional insights and ideas, let us know!

🇧🇪Belgium andreasderijcke Antwerpen / Gent

As @Freya points out, the issue is also present in 2.0.x as the code is basically the same, so the patch from the MR also works on that branch.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

The problem is due to the way hook_preprocess_HOOK works, is triggered for detected templates:

  • For the default templates facets-item-list--searchbox-checkbox.html.twig and facets-item-list--searchbox-links.html.twig, the hook facets_preprocess_facets_item_list() (which adds title) is not triggered.
  • For a copy of the templates in the active theme, the hook facets_preprocess_facets_item_list() is triggered. This causes the title to appear but also results in 2 passes of the template variables through template_preprocess_item_list(). It's the second pass that results in PHP errors.
  • When creating a facet specific variant of a templates in the active theme, hook facets_preprocess_facets_item_list() is not triggered for the template, resulting in the default situation.

Screenshot below showing the hooks for these situations. The scheduler hook doesn't affect the output of the widget, but its presence emphasizes the hook callstack differences depending on the template specificity.

The MR fixes these issues by:

  1. Syncing the facets_searchbox_widget_preprocess_facets_item_list__searchbox_checkbox() and facets_searchbox_widget_preprocess_facets_item_list__searchbox_links() with facets_preprocess_facets_item_list() for the missing title
  2. Preventing double processing of the variables by template_preprocess_item_list()
  3. As a bonus, the cache debugging output has been added to facets-item-list--searchbox-checkbox.html.twig and facets-item-list--searchbox-links.html.twig.
🇧🇪Belgium andreasderijcke Antwerpen / Gent

andreasderijcke changed the visibility of the branch 3365166-cannot-customize-facets-item-list--searchbox-checkbox.html.twig to active.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

andreasderijcke changed the visibility of the branch 3365166-cannot-customize-facets-item-list--searchbox-checkbox.html.twig to hidden.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

andreasderijcke changed the visibility of the branch 3365166-cannot-customize-facets-item-list--searchbox-checkbox.html.twig to hidden.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

When applying the current state of the MR, I'm getting these warnings in a non-paragraphs context:

🇧🇪Belgium andreasderijcke Antwerpen / Gent

This is not an inline entity form issue, it happens on the default entity form as well.

I think the question is, is this a supported case or should setup be reversed?

You cannot make a required field conditionally optional, but you can make it conditionally required.
The latter makes more sense to me, as the given case demands it to be required only when the checkbox is selected.

Also, doesn't field validation complain about the text field being empty when the specific checkbox isn't selected?
I would expect that, unless you have set a default value for the text field. This also make this feel more like a setup problem.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

I've added check to skip removal of items when the there is no content type associated with a menu item, as it without it, it caused the main content link to be removed as well.
With that addition the new "If the content type is not available in the 'content' view 'type' filter." option works as expected on my test case.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

andreasderijcke made their first commit to this issue’s fork.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

I don't understand how patch from #10 could work at all on D10, since it still uses a service that is removed in D10.
Trying applying the same principles to current codebase led me to the following issue.

The setRouteParameter

$url->setRouteParameter('facets_query', trim($pretty_paths_string, '/'));
$url->setRouteParameter('facets_query', $pretty_paths_string);

(see https://git.drupalcode.org/issue/facets_pretty_paths-2974822/-/blob/2974...)

causing the URL to be routed (again), which in turn causes different url generator functions bringing us back to square one.
Currently seeing no way to create an unrouted url object with routeparameters, which makes sense, without diving deep into core.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

Confirming that patch #86 works as expected on a D10.1.7 and D10.2.3.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

The last patch fixes the warning.

@suresh.senthatti What is the reason you have not submitted it for review?

🇧🇪Belgium andreasderijcke Antwerpen / Gent

Patch time_field-3411980-3.patch had missing empty line at the end, causing composer to fail to apply the patch.
Use time_field-3411980-4.patch

🇧🇪Belgium andreasderijcke Antwerpen / Gent

Added option to include the raw value as well, as my case needs it and it seems useful for client applications anyway.
The updated enhancer settings for timerang (similar for time):

This return format of the enhancers is changed from string to keyed array though, having keys 'formatted' and 'raw'.
If inclusion of the raw values is disable, the value of 'raw' is NULL.

Patch time_field-3411980-3.patch corresponds with these changes and the current MR.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

I confirm this fixes the problem.

I do wonder if this can be fixed using https://twig.symfony.com/doc/3.x/filters/filter.html though, so the additional lines with if statements can be avoided. But is just nitpicking.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

Used the current MR state with current dev branch on a D10.1.7 and confirm it works.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

It seems this issue is resolved in 10.2.1 (didn't test in 10.2.0).

🇧🇪Belgium andreasderijcke Antwerpen / Gent

The reason for the observed error, is that the node type entity fetched from the form state isn't updated yet, while the type entity given to the callback already is. Changing that in the code, exposes the underlying problem:

When creating a new node type, is trying to create/update base field values node_keep_form_node_type_form_builder.
When the node type already exists, this will create a base field override.
If the node type is new, it will try to create a new base field definition, which is should not, and for which it is missing info.

Moving this part to a submit handler, run last, fixes the problem.

Please test/review.

🇧🇪Belgium andreasderijcke Antwerpen / Gent

Patch attached corresponding with the current state in the issue fork.
This adds JSON:API enhancers for both the Time and Time Range field, reusing the existing formatting logic to format the output.

To prevent increase of, and reduce already existing code redundancy, I've added a trait to contain the actual formatting logic and the format settings field description. Perhaps more shared stuff like setting default values can be moved here in time.

Because of this trait, I'm not sure if this needs additional tests, at least not for the enhancers in the current version.

I've not added the reverse processing, as it is not required for my case, my spare time is also limited and it opens a new box of questions of what input to support. If supporting this is desired at all. If not, then current version might be good to go.

Just to give an idea what it looks like right now,

1. Regular field formatting vs the enhancers:

2. The enhancer settings:

Production build 0.69.0 2024