Mechelen, πŸ‡§πŸ‡ͺ
Account created on 22 November 2012, over 11 years ago
#

Merge Requests

More

Recent comments

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Do we consider this to be a BC break and as such can it only go into 3.x?

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Added as a supporter

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

I could add keys to UserMailNotifyTest and remove the requiredkey: false from most of them

I agree, this looks like the best way

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ
πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ
πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

borisson_ β†’ created an issue.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

This looks good to me, i don't think we should decrease the size even further.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

We should follow the deprecation protocol we've had in multiple places, use DI, make it nullable and throw a deprecation (and instanciate the service) when the argument is null.

Are we sure we want to do this logging? It might be a lot of additional logs written during config imports?

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Setting this to needs review, we have some test failure, but those are to be expected in the 3.x branch at the moment.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

The schema is complete and the tests don't fail locally when running on 11.x, they do fail when checking out 10.1 and 10.2. They pass on 11.x, so it seems like they are failing because we're using the weight type and that's 10.3+ only. I

think we should have better error message to convey that a type is not found, instead of :weight missing it should say something like: type weight not found. @Wim Leers what do you think, should we open an issue for that? It would've saved me some time debugging to figure that out.

I'll change those to integer so that we are 10.2 compatible for now

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

We'll only tag a new release once we get to a more stable situation, this is still undergoing big changes as it is.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Are we sure we want to mix interface and config translation like this? I don't think we should.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

The indexing in search api happens on a field basis, and it is usually indexed as the anonymous user. This will need a custom processor to translate the node grants into something useful. I'll keep this open to see if there is more interest in this, but I don't think this should be something we include in facets.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

The new config validation test is failing, but I don't understand why. This should be increasing the schema coverage?

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

@phenaproxima, what do you think is the best option here? Do we want to turn this in a meta or do we want to create followups?

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

I think the comments are pretty good; in my opinion this is good to go.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Is there anything wrong in my configuration? Facets should simply work in combination with Ajax, isn't it?

Well, this is a great question, we're currently working facets 3.x which will have better ajax and views support. Ajax support in 1.x and 2.x is severly lacking.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

We should remove this in 3.x

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ
πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

It has been in our faq for a long time that we have issues with view caching, closing this as works as designed for now.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

We haven't seen any support for this issue over the last 2 years, closing this issue as won't fix, but thank you for the idea.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Closing this as won't fix. That patch is part of the 3.x release and we've since worked on that a lot.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Closing as cannnot reproduce, as there were no updates in the last 2 years.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

I think this is still relevant for 2.x, but won't be for 3.x I think? We should discuss this.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

I think this is still relevant for both 2.x and 3.x, because we will still support this widget in theory, setting to 2.x for now.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Closing this as won't fix, because there was no update in the last 2 years.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Looks like this has been fixed in the meanwhile in 2.x and 3.x, thanks for the patch!

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

If we had more maintainers, and specific ones for ranges that would be great. But we don't.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

I think this is an interesting feature request, but I haven't seen any support for it in the last 3 years and I don't think this is something we want to implement at this time. Closing as won't fix for now.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

This should not be related to facets and I assume it has been fixed by now. Please reopen if it is still an issue.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

It makes sense for us to do this. Tagging for the 2.x version.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Closing this as fixed since d11 is coming up soon.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Gotten facets from 65% to almost 90% validatable. Will be trying to fix more of this in the future.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

borisson_ β†’ created an issue.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Added a merge request for this.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

This is a mistake in the comment that still exists, fixing it for 3.x

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Looks like we haven't gotten any more interest in this issue over the last years. Closing this issue in the hopes to clean up the issue queue a bit.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Something like this is very interesting, but we haven't seen any support for this over the last years. Closing this issue as a part of cleaning out the issue queue.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

There haven't been any other people interested in this in the last 5 years. Closing this issue.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ
πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Closing this issue since there hasn't been an update in 6 years.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

I endorse sven for this role, he's a knowledgeable contributor to the community and would be an asset to this group. He also has been an important member of the drupal vzw in belgium without we wouldn't have had such successful events in the past.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

There are still a couple of TBA things in the issue summary, should we update those to be correct?

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

I think we've had change records for a lot of other mark x as fully validatable issues recently, but I'm not sure if we actually need that.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

I agree with this being a good stop gap solution, we can improve the situation further down the line but this is already a good improvement and we have the test coverage to prove that this is an improvement.
I found one extra nitpick.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

I made a mistake, it does apply for 3.x, thank @webflo.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Applied Wim's latest suggestion, it is indeed more clear like this.

πŸ“Œ | Facets | Add gitlab CI
πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Looks like that works, thanks for mentiong that @drunken monkey!
https://git.drupalcode.org/project/facets/-/pipelines/116776

πŸ“Œ | Facets | Add gitlab CI
πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Good idea, I'll attempt to do that.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

This conflicts against 2.x, is it still needed there?

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Saving credit.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Moving this to the 2.x queue, since it probably won't make much sense in the 3.x future.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Closing as won't fix, since it's already been fixed in the module itself. We have a hard dependency on Search API now.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Hard disagree that this should be changed to setters/getters. Especially if the properties stay public.

PS: It seems you (like many others – it's really easy to misinterpret) are confused by the "Issue tags" field. As the guidelines β†’ state, they aren't meant for free text tags related to the issue, but only for specific categorization purposes, usually by module maintainers.
So, if you aren't sure your current usage is correct, please just leave the field empty, especially the name of the module is not helpful at all.

Can we add 2 branches, one for 2.x and one for 3.x?

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ
πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Hopefully resolved the new comments.

πŸ“Œ | Facets | Add MkDocs
πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Looks so professional.

πŸ“Œ | Facets | Add MkDocs
πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Saving credit.

πŸ“Œ | Facets | Add MkDocs
πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Switched the default branch in gitlab. This doesn't have any effect on the recommended version on drupal.org, so that seemed fine to do. Discussed with @strykaizer and @BramDriesen at mountain camp.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Thanks!

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

borisson_ β†’ made their first commit to this issue’s fork.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Thanks @Bram

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

borisson_ β†’ made their first commit to this issue’s fork.

πŸ“Œ | Facets | Add MkDocs
πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

The documentation in https://project.pages.drupalcode.org/gitlab_templates/jobs/pages/ mentions that this is only for the default branch, which is currently 2.x, do we already want to change the default branch @StryKaizer @mkalkbrenner

πŸ“Œ | Facets | Add gitlab CI
πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ
πŸ“Œ | Facets | Add gitlab CI
πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Committed. Thanks!

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

I found one tiny nitpick.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

I'm not sure I agree with wim's remark, it would lead to this same documentation being on all of those instances. I actually think this is rtbc.

I'm not sure how to set the status for this issue now.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Fixed the feedback, rerolled on 11.x and added test coverage for %key. I think this is now ready for another round of reviews.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

we're currently working on the new version of facets, in the 3.x branch to make it work only as views exposed filters. This issue blocks us from having feature parity with the 2.x branch.
We plan on using the https://www.drupal.org/project/configurable_views_filter_block β†’ module, which would also benefit from this patch.
Adding the contrib project blocker tag to reflect that.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ
πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Committed, thanks everyone.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Not sure what the version was originally, now that project is on 5.2.3.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Fixed the first (variable value) remark, the other one is a bit harder to figure out. It's part of the test coverage we created for πŸ“Œ Add new `EntityBundleExists` constraint Fixed , not for this issue.
I don't know how to add the requested test coverage.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Back to needs work to correctly reflect the current state.
I see that you also implemented this fix (or at least the \Drupal::root call) in πŸ“Œ Validate config schema definitions to prevent nonsensical definitions Needs work , does that mean we close this as a duplicate or do we want to keep this open?

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

We should update the IS

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

I think this is mostly better than what we had in the original docblocks, but the wording here is not something we typically use in Core. I think we should only include the ones we know for sure that exist.

Can we find documentation that is similar and copy the style?
Not sure what @joachim thinks about this

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

I agree with #24, we should close this as won't fix.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

I think the update path and update path test are good, and the change to the existing config will prevent this from happening.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

You're right - it is currently also broken, but it is what I had intended to write, to allow something like this https://3v4l.org/HIK4D. Not sure if we want to support this usecase.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Alternative idea: what if we made a custom validator that checked if each individual [a-z][A-Z] character actually yielded output? Because characters that are not supported by PHP will just get printed as-is:

I don't think that'd be right solution, people might want to print something like: "Happened on Fri, Feb 9 2024 around 14:00". That is currently possible, and would no longer be with that solution?

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

I would like to move this back to RTBC and say this isn't truly disruptive, but I actually think it is, we don't know how custom entities look and for them they it would be a change.

Should be simple enough to just update the number in the deprecation

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Sure, adding [a-z][A-Z] is a MUCH simpler option, but that doesn't solve the question asked by @Wim Leers originally:

πŸ€” Shouldn't this have a minimum length of at least one? And … can't we/shouldn't we use a RegEx constraint to ensure this is using one or more of the available letters (to print year/month/day/hour/…)?

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

it's only failing in the javascript tests, looks like a random failure to me. I think this is actually good to go.

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

The current implementation is only on those, but it should do all node types.

See https://git.drupalcode.org/project/search_api/-/tree/8.x-1.x/modules/sea...

πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

Would this allow us to remodel "Search api db defaults" as a recipe that can apply those (or at least some) defaults to all content types on a site?
A high level overview of what that module currently does is:
- Configures a search api server + index
- Adds and configures a view
- Adds a new view mode to basic page + article
- Adds all those bundles to search api

Production build https://api.contrib.social 0.62.1