Belgium 🇧🇪
Account created on 7 January 2016, about 9 years ago
#

Merge Requests

More

Recent comments

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

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

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

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

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

🇧🇪Belgium BramDriesen Belgium 🇧🇪

And fixed :-)

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Difficult to see between all the random failing tests. Triggered a re-run hoping to limit the amount failing tests so we can fix those.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Fixed the two remarks, English is not my native language 😇

Don't really think we need a CR as it was also internal before?

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Updated IS and title, will set it back at RTBC as nothing changed in the code :-)

🇧🇪Belgium BramDriesen Belgium 🇧🇪

For the sake of avoiding further damage to the Drupal brand, please consider reverting to the original front page and moving this redesign exercise to a staging page where only those people who want to help make it better can access it.

That’s how it was before. The new UI was announced at DrupalCon Barcelona and was actually staged already a few weeks before that on new.drupal.org. Everyone was invited to test and post their constructive comments here.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Probably needs a small issue summary/title update

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Fixed the @see to the relevant method.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Looks straightforward enough to me. RTBC

@deepali sardana, please stop making patches. You’ve been told this on like 10 issues already. Yet every new issue you start working on you create a patch.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Hi @deepali sardana

A few remarks:
- Patch workflows are deprecated, you should learn how to use the issue forks workflow. Especially with the GitLab issue migration around the corner. This will probably get you started: https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... also interesting is the DrupalPod workflow.
- Only make edits in scope of the issue. E.g. the changes to the node module or other tests do not belong here in this issue. I would limit the changes of this issue only to the phpdoc in StreamWrapperManagerInterface . Changing to inheritdoc is already done in the other issue.
- I think you omitted some useful information from the code block.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

1. fixed
2. test already refactored
3. test already refactored
4. test already refactored
5. Code doc already refactored
6. test already refactored
7. test already refactored
8. test already refactored
9. trait is refactored and no longer exists
10. fixed

So turns out this is only a minor change to the documentation in the default.settings.php

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Looks ok to me. Still not easily readable though. Maybe converting '_' . $tries to "_$tries" helps, but I'll leave that in the middle for now.

Tests works as expected since the test only run failed.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Really not sure we can write a test for an interface. All tests are passing and code style is fixed.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

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

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Typo and added original participants.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Code can still be improved. Nits from #23 are still not fixed.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

That was messy 😅 should be ok now!

🇧🇪Belgium BramDriesen Belgium 🇧🇪

I think that makes sense :)

🇧🇪Belgium BramDriesen Belgium 🇧🇪

This has not really anything to do with the community section but with the redesign.

https://www.drupal.org/project/drupalorg/issues/3475832 🐛 Feedback on Modern Drupal.org Design Active

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Change looks simple enough :-) but also can't test.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Uploaded JetBrains logo

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Thanks @aylis that looks a lot better! Merged.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

To be honest, I find the documentation not sufficient. If I were new to the module and read

Alters the json response sent from status_dashboard_client.check.

I still have no clue as to what it's doing.

Also code nit, but one should not mix and match snakecase and camelcase in one file.

Production build 0.71.5 2024