๐Ÿ‡ง๐Ÿ‡ชBelgium @BramDriesen

Belgium ๐Ÿ‡ง๐Ÿ‡ช
Account created on 7 January 2016, about 9 years ago
#

Merge Requests

More

Recent comments

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Adding Wim as well ๐Ÿ˜Š

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Added a small snippet which is used on many of the issues being made public.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

bramdriesen โ†’ created an issue.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช
๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช
๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช
๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช
๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช
๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

bramdriesen โ†’ created an issue.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Re #6 We as the security team also observed this. There is a new meta open for this, feel free to chime in. ๐Ÿ“Œ [META|POLICY] Think of a way to make it less easy to become a (co-) maintainer Active

As for the users in this instance who bulk posted the co-maintainer request, there is a tracking issue open ๐Ÿ’ฌ TATA Consultancy Services is bulk posting requests to gain maintainer access to modules Active

Thanks for revoking the rights @legolasbo

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช
๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช
๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

This also fixed a use case on a project of ours where we are trying to filter on a field which is only present on one content type, but we don't want to exclude all the other content types from the results.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

See https://www.drupal.org/project/field_defaults โ†’ for what I feel is the correct solution for this that works for all field types and takes the actual default value of a field into account

If I understand correctly, that will only work if all result items have that field. Not in cases where that field is not present on any of the other content types. I think #2769407 will fix that somewhat.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

8001 is a very old update hook, so very unlikely that that's the issue here. Probably a small mismatch in the schema.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Still seems to be an issue for quite some events, although some are showing images now.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

This is the project_browser issue queue though. So a maintainer here should do that.

Only place where @roderik can do this is in the module he maintains, this issue I guess: https://www.drupal.org/project/samlauth/issues/3413865 ๐Ÿ“Œ Add logo Needs work

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Looks good to me. Saw your comment on the parent issue. Makes sense to add this.

But if any site has already upgraded and still runs into this, the update hook won't fix it.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Hmm good point, in my case I enabled it and exported the config. So I guess we should create a follow up issue then.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

We also bumped into this with a very large index (8 MIL items). I think it makes sense to not do this on config sync.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช
๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

The MR is doing way more as what seems to be needed.

@deepali sardana Only focus on what needs to be fixed in this issue. All the rest would need its own issue.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

The patch is just completely wrong. It's adding what the OP is trying to do in his custom module/theme to Core/Claro...

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Seems like a plain copy paste from the project page...

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

What makes you think the module is not compatible? Your MR would allow lower versions which are probably incompatible.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Eum... I really don't see what would not make it compatible with any release higher as 10.3

The MR is just completely wrong.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

I wanted to say the real solution would be to be able to create a project without git repository. But with the Drupal.org issue queue becoming gitlab issues, that will become redundant before it's even implemented.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Don't think there is an easy way to distinguish code or none code projects as the "node" this is created under is probably module project. In the end, any project being code or none code get's a space on GitLab, so technically, it could contain code. https://git.drupalcode.org/project/artificial_intelligence_initiative

I guess the fastest way is to opt-in into the advisory, even though there might never be a single line of code or stable release being tagged which would enlist it as covered.

Just wondering, why is this not a page on the documentation system, and the "project" just for tracking issues/credits? That way, one would not need to market the project page but the documentation page instead.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Let's leave this one at NR for @legolasbo to decide as the request was granted.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

It seems weโ€™d just need to remove the โ€œwhen used in a namespaced fileโ€ part?

Probably yes, and update coder/phpcs to check for this.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Attaching screenshot of Drupal Core (11.x) code search. Looks like many things have been fixed already between Drupal 9/10 to 11 as I can find many more occurrences in our projects (might come from patches as well of course).

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

This has been a discussion point in the MR's of our team for our projects. Not so much about what is in the IS but more about the global \Drupal:: calls in none name-spaced files (e.g. .module).

I would usually write my code with the leading backslash.

$facet_manager = \Drupal::service('facets.manager');

But then a team member would come in saying the leading backslash is not required as we're not in a namespace. Looking at Drupal core, I can see a mix of both being used, so I think it's good if we could standardise this as a standard.

Core example where it is done with a \ : https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/autom...

Core example where it is done withouth a \ : https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/toolb...

Happy to say thought that there are not that many cases without the \

PS: I find coding standards difficult English to read ๐Ÿ™ƒ so if this is not the same thing we're discussing let me know and I'll create a new issue.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช
๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช
๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

bramdriesen โ†’ created an issue.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

On a positive note, by taking maintainership of modules which are not activity maintained, we are trying to play an active role in making such modules more responsive to demand for changes.

Sorry to say this, but bulk posting requests to become maintainer is not the way to go in my opinion. This was being posted on active maintained modules as well, none of them which I checked were abandoned. There are better ways to contribute instead of asking to become maintainer of a module without even having contributed to a single issue on set module.

They are neither bot nor spam

Bulk posting the same request over and over does fall under spam.

If their experience on Drupal.org is only the question , then i can have their mentor take on this.

Their experience is not shown on an empty user profile either. I'm not questioning it, but those profiles listed in the IS do not show anything.

FWIW, you do not need to be a maintainer in order to contribute. So I would highly recommend you stop pushing your mentees in that direction.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Thanks @nicoschi, decided to merge this since none of the other maintainers objected to it since #15. Will tag a new release shortly.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช
๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Merged, thanks Dave!

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

This hasn't really anything to do with project ownership transfer, which is only really used for abandoned modules if I'm not mistaken. But it for sure is related ๐Ÿ™‚.

I'm following ๐Ÿ“Œ Add ability for organziations to manage/approve contributors(employees) Active so also related, but no the issue I had in the top of my mind ๐Ÿ™ƒ I'll check tomorrow if I can find it.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Do I believe we as a community have a right to overrule a maintainer: No.

That's 100% correct. And it's no difference from let's say maintaining an NPM package on GitHub. BUT I think we should at least try to find a way to make sure a maintainer is trustworthy before being added as a maintainer to a project. (see referenced META issue in the securitydrupalorg project).

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Re #4

2. Are they part of a company on the community radar for concern?

I don't think that's really a trustable check. I can create a new account and register myself as an Acquia employee on Drupal.org. There is no check involved here. I remember seeing an issue about this somewhere to allow orgs to enable a setting that they have to verify users.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Adding a few more issues. Think I have spotted 6 different modules so far where a user was added as maintainer without any second thought.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Spotted another user in one of the threads.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

@legolasbo You might want to re-consider granting a random and new user maintainer access. This module has 10K installs, @peelas02 has no single post or contribution made for this module besides the D11 bot issue. This raises some security questions as well that this is being granted so easily.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Change looks straightforward enough! But should use $this->t() I think

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

@g089h515r806 You might want to re-consider granting a random and new user maintainer access. This module has 35K installs, @peelas02 has no single post or contribution made for this module. This raises some security questions as well.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Adding #30 to the remaining taks. Something a site admin can do I presume.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

I created a quick MR to fix the issue for our use case. What I noticed:
- When using a view (search API results) with AJAX, results are 0 when reaching the IF check
- The #view element is not showing all settings correct, E.g. AJAX is set to false while it is enabled on the view.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Thanks! I guess this could become a meta issue then. Created a follow up issue as the rendering stopped working on our site.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

bramdriesen โ†’ created an issue.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

@jradhak why are you not using merge requests/issue forks like you should? You're directly committing on the dev branch without any reference to the related issues. This way it's very difficult to track back the original issue.

Also, why are you dropping D9? I know it's EOL, but if there are no deprecations there is no real reason for that.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

@jradhak Why are you committing changes to the dev branch before they are reviewed?
Why are you dropping D9?
Why are you not using merge requests but doing commits directly?

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

100% agree with what you said Catch

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

@deepali sardana Your MR introduces code sniff issues. It's not recommended to just blindly copy over what might fix a given issue from the issue summary. Also I don't see any need to wrap it in round brackets. I think the only thing relevant here is the string conversion, which might actually crash if the value is NULL. This might need a fix higher up.

But given your track record, you probably will not look again at this issue and just continue on the next one.

Fixing some meta data as I go.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

I don't think the idea was to archive the DDEV WSL2 page. At least I don't see @catch or the IS mentioning that page.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช
๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

I think tagging a new beta would also help to get some adoption.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Attempt at helping to clean this up. Also fixed a PHPCS issue on the last commit for the 11.x branch.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

bramdriesen โ†’ changed the visibility of the branch 3498170-automatic-conversion-of to hidden.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

bramdriesen โ†’ changed the visibility of the branch 3498170- to hidden.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

bramdriesen โ†’ made their first commit to this issueโ€™s fork.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

It seems to reference "https://www.drupical.com/f/none.jpg" for all events actually, which means not even the placeholder is working ๐Ÿ™ƒ

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

bramdriesen โ†’ created an issue.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

For #13 and #11

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Large images were also reported in #105 there it was suggested to convert to SVG which I think is a better approach. ๐Ÿ™‚ But I ain't a front-end developer.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

The logo got committed to the 2.x branch

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Nice, only thing left would be to change the default branch to 2.x

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Extending or using a badly named enum or class will not throw an error. This sniff only checks the declaration of the enum.

Correct, but fixing them would break things

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Would also be quite disruptive in contrib space where on could be extending off a class or using an enum that is badly named.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Re #192 Note that I have nothing to do, at all, with the re-design or the development of it.

The argument that "it was announced at Barcelona" is reminiscent of Author Dent's house being demolished and the council claiming he had been informed because therr was a note in a locked file cabinet in a basement behind a door that said "do not enter".
....
comments that read basically as "you had you chance when didn't fell you".

You are pulling my comment out of context. My comment was as a reply to a comment that the redesign was "just" published without it being staged and without time for the community to respond and give feedback.

I have been doing Drupal News videos and paying very close attention to the developments of Starshot for thr past few motnhs and yet I missed that news entirely even after watching Dries's key note from Barcelone.

It was mentioned here with the preview link below it: https://youtu.be/nhPiL4g972A?si=VkXZoYImcGMZVOE9&t=1088
The brand redesign itself was apparently even shown in Portland (I did not watch that Driesnote ๐Ÿ™‚).

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

@deepali sardana Like mentioned on many issues already to you. Patch workflows are deprecated. Please create a MR.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

bramdriesen โ†’ changed the visibility of the branch 3496738-moving-moderationstatus-to to hidden.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Correct ๐Ÿ™‚ a core maintainer can probably verify best if the current test is sufficient. But to me it looks to be the case. Latest test run is 100% green as wel after a random test failure.

So I think we can actually set this as RTBC.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Patch workflows are deprecated. Patch is introducing many PHPCS issues. Seems to be generated by AI judging by the sudden addition of inline comments which is typical for AI.

Also, is what is in the IS actually being fixed? I don't see any label related changes.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

I have the feeling this MR will fundamentally break the module.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Maybe we can just add a test (if it's not existing yet) that when we create a file using the Editor module, it ends up being permanent on the end. Not specifically checking if setPermanent is called twice, but just the end result.

Also sorry for the commit and not applying the code suggestion, totally missed that.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

NW is more appropriate.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Quick review code wise, there are some things which are not correct. Also still needs tests, and currently tests failures.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

MR looks clean. Just wondering if the "description" which was added in the test is really needed, unless it's required by default of course.

Test run looks good and the test only is failing as expected.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

MR does not look okay. Started from the wrong branch? Wrong target branch?

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

We have failing tests, and still need new tests. The MR also introduces PHPCS issues.

๐Ÿ‡ง๐Ÿ‡ชBelgium BramDriesen Belgium ๐Ÿ‡ง๐Ÿ‡ช

Added a small nit, but will leave it at NR

Production build 0.71.5 2024