Belgium 🇧🇪
Account created on 4 June 2014, almost 11 years ago
#

Merge Requests

More

Recent comments

🇧🇪Belgium tim-diels Belgium 🇧🇪

This will need more information to see what is wrong.

Can you verify if the correct packages repositories are included to fetch the modules from Drupal?

🇧🇪Belgium tim-diels Belgium 🇧🇪

I added the patch as MR and manual testing seems to solve the issue and does not break on other parts. But maybe I'm not testing all cases.
Putting this to needs review for others to review aswel.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Added screenshot of error for direct idea of issue

🇧🇪Belgium tim-diels Belgium 🇧🇪

I did not find a quick solution to have the messages in an own container. So for now I added them to the global container as we're going to use Toastify to print the messages in a better way. If someone wants to improve this, feel free to do so.

🇧🇪Belgium tim-diels Belgium 🇧🇪

I added some console logs to make it clear what is happening. Also did a check on the address if not empty and then don't call the geolocation provider.

This can however be improved and show a message in the website itself. I was looking at https://www.drupal.org/node/2930536but that will print the message on top of the webpage and is not ideally. The message should appear inside the fieldset? I would suggest on adding a message container in the fieldset and attach the messages there. But other ideas are welcome on how to tackle this.

🇧🇪Belgium tim-diels Belgium 🇧🇪

I provided some code changes already to No visual message when geolocate returns nothing Active that handles this. Lets focus on the work done there to continue.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Added support for the module. Not sure if it needed to be in a submodule or just in the base. So didn't add any tests yet. But shouldn't be that difficult to add. But first would like a review of a maintainer on whether it needs to be a submodule or not.

🇧🇪Belgium tim-diels Belgium 🇧🇪

tim-diels created an issue.

🇧🇪Belgium tim-diels Belgium 🇧🇪

This is required to get the module working.

🇧🇪Belgium tim-diels Belgium 🇧🇪

As per last 2 comments, this will need some adjustments.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Hey Berdir, thanks for shiming in. I didn't feel like putting this on a status that is different then before without any reason given. So thats why I want back to the previous status as I didn't test it myself. But fair enough for me if you want to put it on RTBC. I did see indeed that the pipelines are failing on more MRs and are indeed not related.

🇧🇪Belgium tim-diels Belgium 🇧🇪

@divyansh.gupta no need to double verify as this has been verified before. And setting something to RTBC when the pipelines fail is not correct. So setting this back to NW.

🇧🇪Belgium tim-diels Belgium 🇧🇪

As there is a patch, this can be set on needs review.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Added the patch as MR and tested it out and it seems to work for what is described. The pipelines are failing, but after looking at them it seems unrelated to this change.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Let's create a MR for this so the pipelines are triggered and we can see if there is work needs to be done.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Please provide patches as MR as that will be easier for review and also triggers the pipelines for quality assurance and testing.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Please provide patches as MR as that will be easier for review and also triggers the pipelines for quality assurance and testing.

🇧🇪Belgium tim-diels Belgium 🇧🇪

I'd like to cast a vote for:
- Joris Vercammen (borisson_)
- Marine Gandy (Mupsi)

🇧🇪Belgium tim-diels Belgium 🇧🇪

I can't close the MR so left issue open for now. Can you close it?

🇧🇪Belgium tim-diels Belgium 🇧🇪

Hi Jurgen, thanks for the great support here. I finally was able to come back to this and you're absolutely correct in your explanation to use the plain: token. Thank you so much for the help!

🇧🇪Belgium tim-diels Belgium 🇧🇪

Hi, could you please make a release?

🇧🇪Belgium tim-diels Belgium 🇧🇪

I'm reopening this as I got the same issue. This is due to the services file and some other files are added as .example files.
Suppose the maintainers expect us to put them in a custom module to use those? But there is no documentation about this, so would like some insight from a maintainer on why the files are added as .example and how to continue.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Hi mohamad, thanks for the quick response. Can you also add it as a (download) release (2.0.x) so we can tag the issues to the version? If you don't know what I'm talking about, just let me know. You can also ping me on slack at tim-diels

🇧🇪Belgium tim-diels Belgium 🇧🇪

Hi mohamad, thanks for the quick response. Can you also add it as a (download) release (2.0.x) so we can tag the issues to the version? If you don't know what I'm talking about, just let me know. You can also ping me on slack at tim-diels

🇧🇪Belgium tim-diels Belgium 🇧🇪

As mentioned in #3522879: Support Elasticsearch Connector 8.x this will result in a breaking change and should be done in a major release.
Why is this is breaking change? We should rename the current method to msearch to match the elastic documentation and the new method should use the current search method (but changed ofcourse).

🇧🇪Belgium tim-diels Belgium 🇧🇪

In my enthusiasm I also added the default search method. This is actually a breaking search for people extending the controller.
Therefor we should extract the method. I created a new issue for this #3523543: Add search method .

Also the support for Elasticsearch Connector 8.0.x should be done in a major release. So can you please create a new branch for this so we can set the MR to the new branch?

🇧🇪Belgium tim-diels Belgium 🇧🇪

Added a display widget, could use some improvement maybe as this is what I currently needed.

🇧🇪Belgium tim-diels Belgium 🇧🇪

tim-diels created an issue.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Added it to a MR for easier follow up. Still need to be tested.

🇧🇪Belgium tim-diels Belgium 🇧🇪

tim-diels made their first commit to this issue’s fork.

🇧🇪Belgium tim-diels Belgium 🇧🇪

I'm sorry for answering so late, but can you provide a complete example on what you try to achieve so it is testable?

🇧🇪Belgium tim-diels Belgium 🇧🇪

Ok code did not change a lot, can you point out what is still needed?

🇧🇪Belgium tim-diels Belgium 🇧🇪

Hi Wouter, thanks for the report and the patch. Could you see if 🐛 When there's a path that is a subpath of trans_path, the getOutboundPath function messes up paths from other places Active fixes the issue? If not, you can use it as base to create a test that fails and then a fix?

🇧🇪Belgium tim-diels Belgium 🇧🇪

Thanks a lot for this great work! Looks wonderful and tested and works as expected. Did some minor changes. Feel free to question these changes and explain why you would need them. I don't see the value of them at this moment.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Hi @steinmb, thanks for the MR, but its still in draft, so not sure if its finished? The code will change a lot with the other issue. So I'm thinking of getting back to this after the other issue has been tested and merged and see what needs to be done here? What you think?

🇧🇪Belgium tim-diels Belgium 🇧🇪

Hi @steinmb, sorry for not being to active on this module. I've been trying to follow up on most of the modules I maintain, but most of them I did not build but got maintainer to get thing forward. I understand the frustration or irritation when things keep hanging. I'll review this module in the next days and see if I can get everything tested.

🇧🇪Belgium tim-diels Belgium 🇧🇪

I added support for Elasticsearch Connector.

🇧🇪Belgium tim-diels Belgium 🇧🇪

@dhaval_panara please do not use mr diff links in your workflow. This is very dangerous! Please read https://github.com/cweagans/composer-patches/issues/347

🇧🇪Belgium tim-diels Belgium 🇧🇪

tim-diels changed the visibility of the branch 3366187-gin-theme-compatibility to hidden.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Found an issue when using elasticsearch to index nodes. So will need to add an extra check.

🇧🇪Belgium tim-diels Belgium 🇧🇪

As there is no dev release, I targeted the 8.1.1 version and used the 8.x branch as thats where the latest code is added.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Also with the pipelines running now, there is more work to do.

🇧🇪Belgium tim-diels Belgium 🇧🇪

I rebased both MRs but for the 2.x I rebased on the 2.3.x so that MR needs to be changed to 2.3.x instead of 2.2.x. I4m not able to do this, anyone else have this option?

🇧🇪Belgium tim-diels Belgium 🇧🇪

Hiding the patches files as the current approach is to use MRs. There are 2 MRs that seem to solve the issue and patch from #41 looks the same as MR157.

I've tested MR156 and that provided the tokens I was looking for.

But it seems no pipelines are running? I'll rebase and see if that triggers the pipeline.

🇧🇪Belgium tim-diels Belgium 🇧🇪

This is a big blocker for normal composer workflow. We're using this for a month now and looks like its working as expected and does not create any issues.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Trying to reproduce the issue in the first place, but not able to do so. Is anyone else able to reproduce this?

🇧🇪Belgium tim-diels Belgium 🇧🇪

Added both patches in 1 MR. The MR is the latest patch from #3
Will test what msypes describes

🇧🇪Belgium tim-diels Belgium 🇧🇪

tim-diels made their first commit to this issue’s fork.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Pinged both maintainers on slack to see if they can have a look.

🇧🇪Belgium tim-diels Belgium 🇧🇪

You should use context in your custom code to tell Drupal the string needs to be translated in a given context. See https://www.drupal.org/docs/7/api/localization-api/string-context
This has nothing to do with this module I think.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Thanks for the work @sourabhsisodia_!
But as you see there is still some fine tuning needed. Would you mind picking this up again? If not, someone else can pick this up after 48 hours.

🇧🇪Belgium tim-diels Belgium 🇧🇪

I tried looking where to get the :raw token, but thats not available except for the date field or the webform module that provided their own integration.

I tracked it down to having the entity label set from the group title/label with special characters return a html encoded string. We do not need an extra plugin to get the raw value but maybe we can do htmlspecialchars_decode on the entity label set from a token?

So in the classNewEntity in the method execute we can replace the following:

if (isset($entity_keys['label']) && isset($config['label'])) {
  $values[$entity_keys['label']] = trim((string) $this->tokenService->replace($config['label'], [], ['clear' => TRUE]));
}

with

if (isset($entity_keys['label']) && isset($config['label'])) {
  $values[$entity_keys['label']] = htmlspecialchars_decode(trim((string) $this->tokenService->replace($config['label'], [], ['clear' => TRUE])));
}
🇧🇪Belgium tim-diels Belgium 🇧🇪

This issues seems to be outdated as the Gitlab CI is now running instead of Drupal CI?

🇧🇪Belgium tim-diels Belgium 🇧🇪

Yes the cache clearing process needs to be looked at as I have similar issues.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Created MR for 3.x based on the work done above.

@mlncn I like the suggestion you make, but it will take more time and is more complex to get this done. Maybe split that to a new feature request?

Test is failing probably because the cache is not getting cleared properly to hide the link.

But to me this also seems to be incomplete and it should however take care for individual entity types. Now its a global setting but only hiding on the admin/content page. So either have a global setting to hide it everywhere (not a fan of this) or have the setting per entity type.
So I guess this code will need some refactoring to have this as an option for every entity type.

🇧🇪Belgium tim-diels Belgium 🇧🇪

tim-diels changed the visibility of the branch 3.x to hidden.

🇧🇪Belgium tim-diels Belgium 🇧🇪

tim-diels made their first commit to this issue’s fork.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Ok then we can set this to needs review. Thanks.

Production build 0.71.5 2024