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

Merge Requests

More

Recent comments

🇧🇪Belgium tim-diels Belgium 🇧🇪

tim-diels created an issue.

🇧🇪Belgium tim-diels Belgium 🇧🇪

I see in 2.x that there is a new approach taken. Additional flexibility is built in to that controller allowing developers to override it and customize the behavior/display in new ways. See https://www.drupal.org/project/section_library/issues/3505852#comment-15 Use view mode in 'import from library' and links template suggestion Active ... for an example.

This 2.x branch makes this adjustment obsolete. We wont go further with the approach from this issue. Therefor I'll close this issue. Thanks for the great work!

🇧🇪Belgium tim-diels Belgium 🇧🇪

Can this please be merged?

🇧🇪Belgium tim-diels Belgium 🇧🇪
🇧🇪Belgium tim-diels Belgium 🇧🇪

Looks legit to me. I'll commit this.

🇧🇪Belgium tim-diels Belgium 🇧🇪

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

🇧🇪Belgium tim-diels Belgium 🇧🇪
🇧🇪Belgium tim-diels Belgium 🇧🇪

I've tested this and was able to reproduce the error and the fix seems correct to me.

🇧🇪Belgium tim-diels Belgium 🇧🇪
🇧🇪Belgium tim-diels Belgium 🇧🇪

We should set the minimal D10 version to 10.2. For the rest it looks ok. I'll commit this.

🇧🇪Belgium tim-diels Belgium 🇧🇪

What is needed to get this comitted?

🇧🇪Belgium tim-diels Belgium 🇧🇪

Just adding the patch in a MR. Didn't test yet.

🇧🇪Belgium tim-diels Belgium 🇧🇪

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

🇧🇪Belgium tim-diels Belgium 🇧🇪

But do we need this if the related issues are solved? I don't have the time to investigate the best way forward. But if anyone knows, please let us know.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Created a MR and going to hide all patch files.

🇧🇪Belgium tim-diels Belgium 🇧🇪
🇧🇪Belgium tim-diels Belgium 🇧🇪

Can be reviewed.

🇧🇪Belgium tim-diels Belgium 🇧🇪

tim-diels created an issue.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Thanks for the reply, going to close this as it is not due to this module itself.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Hi weseze, thank you for creating this feature request!

Could you elaborate a bit more on the first paragraph? I'm not completely follow what you're doing.

On the second hand patches are generally not used anymore. The best way forward is to create a MR containing the patch and remove the patch. This will allow any maintainer to have a better overview of code change, rebases can be done easily and code checks and test can be run automatically.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Hi Ian, thanks for the issue. I’m one of the co maintainers of this module. I’ve had little time but I’m going to look into the issues. I’ll give an update to this issue. If any of the open issues can use a review, please be free to do so and mark it as reviewed. I’ll try to look at it as soon as possible

🇧🇪Belgium tim-diels Belgium 🇧🇪

Hi Finex, thanks for the report. But can you explain in steps how to reproduce this error? So that someone can reproduce it and start working on a solution?

I would like to point you to the Drupal.org documentation about creating issues:
https://www.drupal.org/community/contributor-guide/reference-information...

And more specifically:
https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-...

Can you please update the issue?

🇧🇪Belgium tim-diels Belgium 🇧🇪
🇧🇪Belgium tim-diels Belgium 🇧🇪
🇧🇪Belgium tim-diels Belgium 🇧🇪
🇧🇪Belgium tim-diels Belgium 🇧🇪

No problem. Thank you also for the work on this. I'll be working on a release this weeks.

🇧🇪Belgium tim-diels Belgium 🇧🇪
🇧🇪Belgium tim-diels Belgium 🇧🇪

It would be also good to see if the packs can be (visible) grouped so the end user knows the icons are in different packs?

🇧🇪Belgium tim-diels Belgium 🇧🇪

I'll provide a Mr for this

🇧🇪Belgium tim-diels Belgium 🇧🇪

Thanks for the issue report and the extra effort by providing good insights on what could be wrong. I'm not sure what the default situation could be. I'm not sure the edit for should be specified. As you suggest the module could try to look for the edit form if specified, otherwise fall back to the default? Is that maybe how Drupal core does it also?

🇧🇪Belgium tim-diels Belgium 🇧🇪

Why changing status to needs work? Can you explain what the next steps would be, if any?

To me, this is still RTBC

🇧🇪Belgium tim-diels Belgium 🇧🇪

This will need an update using the latest template.

🇧🇪Belgium tim-diels Belgium 🇧🇪

IT would help if you provide more information in the issue summary so others don't have to look into the code to understand what the issue is about. The ticket template is there for a reason.

🇧🇪Belgium tim-diels Belgium 🇧🇪

As there's a patch, this can be set to needs review.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Hi Plach, I created the issue before the biggest blocking issue was resolved.

I do understand that you are hesitated to allow other persons as maintainer. Force there is no big must at this moment to maintain extra modules. I just wanted to have the new release done. As that’s done, for me it’s ok to cancel this request.

I am already happy that the module exists and that there was a reaction.

I will do some contributions if needed, but no need to make me a maintainer if there’s follow up.

For me this can be closed.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Since the other issue is fixed. This can be reviewed also.

🇧🇪Belgium tim-diels Belgium 🇧🇪

Here we go, MR created.

🇧🇪Belgium tim-diels Belgium 🇧🇪

tim-diels changed the visibility of the branch 3282292-support-for-entity to active.

🇧🇪Belgium tim-diels Belgium 🇧🇪

tim-diels changed the visibility of the branch 3282292-support-for-entity to hidden.

🇧🇪Belgium tim-diels Belgium 🇧🇪

The patch from entity browser and this one together works for me. Thanks!

🇧🇪Belgium tim-diels Belgium 🇧🇪

@arnaud-brugnon this doesn't work for me. The patch in #4 did work tho.

This needs to be in a MR.

🇧🇪Belgium tim-diels Belgium 🇧🇪

@er.garg.karan , you opened a MR for 4.0.x but the branch is still 3.0.x. Can you please update that? I'm not able to do it for you...

🇧🇪Belgium tim-diels Belgium 🇧🇪

I added patch from #8 to a MR. Hiding all patch files as not relevant anymore.

Credits go to zhaoyao@ciandt.com and Oscaner forthe work done.

Maybe we need to define next steps in how to proceed in this?

🇧🇪Belgium tim-diels Belgium 🇧🇪

Hiding patch from #9 as the patch provided there is exactly the same as #8 except that the paths to the files are changed and incorrect. So lets add #8 as a MR to correctly follow up and let the pipelines run to see if there is still work to be done.

🇧🇪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 🇧🇪

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.

Production build 0.71.5 2024