🇧🇪Belgium @BramDriesen

+1 But this is key!

The commit author address is a conscious choice by developers that should be respected by default.

As described on your git instructions page: https://www.drupal.org/user/UID/git → . One has the choice to use an anonymized address.

Really nice! I haven't tested yet, but I presume that the new git commit message is ready to use?

I believe there still is the unhandled comment from @benroy on the backport MR which also applies to the 11.x MR.

Then there is the open question in #72 and open comments on the 11.x branch. Will leave it at NR for now so it hopefully get's some attention from the correct people as well to help decide on this.

@andileco What's the status of your work? 😊 can this be reviewed?

I came here because of a slack discussion. If a review can help land an issue I will do so (a bit in the light of the boy scout rule). I actively try to avoid working on novice issues and even create novice issues for the modules I maintain for others to pick up 🙂. But reviewing a core issue in my perspective might fall outside of a novice task. Especially one like this where one might think you can just delete the two empty test cases.

Looks good to me. Also referencing the issue where those test cases where initially added and why they exist.

core/modules/system/tests/src/Functional/GenericTest.php is indeed the one to keep.

Just to highlight why this was changed back to active, this should not be a new submodule in core.

You might want to read this before working on Core issues: https://www.drupal.org/community/contributor-guide/reference-information... →

Updated and also on the event page.

There are no merge conflicts, the GitLab diff is for 11.x (dev). Are you on the dev release of D11?

Also, you should use .diff and not .patch.

bramdriesen → created an issue.

bramdriesen → created an issue.

This seems major as it's fundamental for the module.

The MR is a blind re-roll without reading through the issue what needs to be actually done.

I'm also still not sure if we need this, maybe it has already been fixed? I guess this can be manually tested (without the patch) to see if there is a conflict with the conflict module.

bramdriesen → created an issue.

bramdriesen → created an issue.

And released! https://www.drupal.org/project/time_field/releases/2.2.0 →

Finally was able to test this manually, this works like it's expected to! Thanks again @tame4tex

I think this needs a lot of clarification. It sounds more like a paragraphs issue. On the other hand I have no reasons to believe paragraph fields are untranslatable.

@_pratik_ why update an outdated patch when there is an issue fork with more changes? Why upload a patch when the patch workflow is deprecated?

Found it.

@astonvictor Please link the issue where the deprecation was fixed.

📌 Automated Drupal 10 compatibility fixes Active only changed the info.yml file

More strange behaviour which might be automated or AI assisted in some way.

RTBC'ing an issue which is merged and fixed for over a year: https://www.drupal.org/project/select2/issues/3271205#comment-16191852 🐛 Removing selected option sometimes needs multiple clicks RTBC

Reviewing old/outdated patches when there are newer versions/MR's available:
- Yes, same issue: https://www.drupal.org/project/select2/issues/3271205#comment-16191852 🐛 Removing selected option sometimes needs multiple clicks RTBC
- https://www.drupal.org/project/drupal/issues/3301239#comment-16190176 🐛 PoStreamReader::readLine() throws an error on module install Needs work

Re #54, why are you testing/reviewing a very old patch (#7) when there are rebased patches and a merge request? This feels like lost effort that could have been spent better.

Also "Works for me" is not a Drupal.org status. We have "RTBC (Reviewed and Tested By the Community). But since you're reviewing old patches, it's not really ok to make that change here after your review.

RE #4, what did you test? There is no patch #2, the patch you are referencing (3323974-media-link-blank-target-11.x.patch) does not exist, nor here, nor in issue #3323974 which I guess is not even closely related to this one.

Did you use AI to generate your comment? If yes, be aware this is covered under the credit abuse policy. https://www.drupal.org/drupalorg/docs/marketplace/abuse-of-the-contribut... →

Re #31, what did you actually test? You're referencing the patch from #2, but also the MR  !11081 which contains more changes as the patch...

bramdriesen → created an issue.

Very minor change, but MR looks good.

I guess some of this can be broken down into separate issues?

Usability team would indeed be better. Thanks @quietone!

I think we're going to keep go in circles until a core maintainer helps with suggesting the correct string/wording changes for this.

bramdriesen → created an issue.

@rahaf albawab patch workflows are deprecated. Please update existing issue forks... You provided no info or interdiff so it's unclear what you fixed, if anything. Hiding patch for that reason.

+1 For #9, I think this should be made very clear.

The discussion for adding a label for "AI generated" modules is separate discussion happening elsewhere.

Is this about "modules entirely written by AI without much human interaction which might contain security issues" that need the label.
OR
"Modules that integrate with AI tool X/Y written by a human"?

For the first one, I think we don't need a module category but a label which can be seen as a disclaimer (similar to thingiverse/printables for AI STLmodels).
For the second one, a module category would work (e.g. for modules under the AI initiative)

MR looks good now, thnx @xjm :-).

And thanks Sam!

I need to manually test the upgrade path on an existing site with data. Code wise this looks really good though, so I'm quite confident this will be ok.

And thank you for your work @tame4tex

No update for over a year. Please re-open if still relevant.

This was fixed in 🐛 AssertionError: "General" must be defined in MODULE_NAME.field_type_categories.yml in assert() (line 183 of core/lib/Drupal/Core/Field/FieldTypePluginManager.php). Active . Assigning credits for the work done here.

Thanks everyone. Merged and will tag a release shortly.

The MR description screams AI, so does some of the text changes.

Not all suggestions are properly fixed. Also added one extra comment about the "hosting provider" thing as I'm not sure that is still relevant this day of age.

+1 Would also recommend creating a change record for this.

This could use a change record for 2.x this has already broken pipelines.

I guess when one goes another one pops up :-) there are also other tools like Datadog and uptimerobot that can do some things, but not all.

Some others I found:

https://docs.inspector.dev/guides/drupal

https://www.drupal.org/project/ohdear_integration →

Removed leading space from title.
Proofread all strings and added one extra comment to the MR.

Landed on this issue through the change record. I updated that one so the version is removed to be more clear it's not yet published.

Issue could use an issue fork instead of patches :-).

Checked a few other recent changes and fairly certain we need to keep the module and use - ? when there is no submodule maintainer.

Should the whole block be removed? And not only the maintainer like the other submodules?

Example

Content Translation
- ?

Probably could use some tests, but this is ready to go!

No new features will be added to 8.x-1.dev. This includes D11 support.

Please upgrade to 2.x as it also contains a lot of bugfixes and new features.

bramdriesen → created an issue.

The page I linked is indeed about the credit system, but this is part of a larger discussion: https://www.drupal.org/project/site_moderators/issues/3332699 🌱 Do we need a policy on AI-generated content Active

You can't opt out of the credit system as far as I know.

Just make it clear on your module description that it's entirely written with LLM language models and might contain security issues. There is no defined format or message to display on your module. Here is an example of another module that kicked up some dust: https://www.drupal.org/project/static_node →

@maxilein Note that publishing AI generated modules without disclosing it's AI generated is against the policy and is considered as abuse.

https://www.drupal.org/drupalorg/docs/marketplace/abuse-of-the-contribut... →

Your module description and code clearly show signs it's just AI gibberish.

Your module is now on my personal blacklist.

Hmm that should not happen. I think the whole if/else section could be simplified to also improve readability.
And o boy, what a bad variable naming happened when this module was created 😅
I think a small description for what "Skip Display to own activities" does would also help.

jpvos → credited bramdriesen → .

xjm → credited bramdriesen → .

I think for my use case it's as simple as:
- Create a view with a page
- Add a menu link to set page
- Try to visit the view page

I'm not testing this on a vanilla project.

Ok this is ready for review again. Tests all pass :-)

This is the underlaying issue: The request you're passing in is the current request, not a request for the views route.

I also have this issue, however my stacktrace is a bit different because it's about a ViewPage.

Error: Call to a member function hasOption() on null in Drupal\views\Routing\ViewPageController->getRouteArguments() (line 101 of core/modules/views/src/Routing/ViewPageController.php).
Drupal\views\Routing\ViewPageController->getTitle()
call_user_func_array() (Line: 58)
Drupal\Core\Controller\TitleResolver->getTitle() (Line: 433)
Drupal\menu_breadcrumb\MenuBasedBreadcrumbBuilder->addMissingCurrentPage() (Line: 348)
Drupal\menu_breadcrumb\MenuBasedBreadcrumbBuilder->build() (Line: 85)
Drupal\Core\Breadcrumb\BreadcrumbManager->build() (Line: 73)
Drupal\system\Plugin\Block\SystemBreadcrumbBlock->build() (Line: 171)
Drupal\block\BlockViewBuilder::preRender()
call_user_func_array() (Line: 113)
Drupal\Core\Render\Renderer->doTrustedCallback() (Line: 870)
Drupal\Core\Render\Renderer->doCallback() (Line: 432)
Drupal\Core\Render\Renderer->doRender() (Line: 504)
Drupal\Core\Render\Renderer->doRender() (Line: 248)
Drupal\Core\Render\Renderer->render() (Line: 484)
Drupal\Core\Template\TwigExtension->escapeFilter() (Line: 244)
__TwigTemplate_8470b504d89b71c3df10927dbb9dcb9a->block_breadcrumbs() (Line: 432)
Twig\Template->yieldBlock() (Line: 118)
__TwigTemplate_8470b504d89b71c3df10927dbb9dcb9a->doDisplay() (Line: 388)
Twig\Template->yield() (Line: 344)
Twig\Template->display() (Line: 359)
Twig\Template->render() (Line: 51)
Twig\TemplateWrapper->render() (Line: 33)
twig_render_template() (Line: 348)
Drupal\Core\Theme\ThemeManager->render() (Line: 491)
Drupal\Core\Render\Renderer->doRender() (Line: 248)
Drupal\Core\Render\Renderer->render() (Line: 484)
Drupal\Core\Template\TwigExtension->escapeFilter() (Line: 108)
__TwigTemplate_a879401966f69f40ede102b368340c72->doDisplay() (Line: 388)
Twig\Template->yield() (Line: 344)
Twig\Template->display() (Line: 359)
Twig\Template->render() (Line: 51)
Twig\TemplateWrapper->render() (Line: 33)
twig_render_template() (Line: 348)
Drupal\Core\Theme\ThemeManager->render() (Line: 491)
Drupal\Core\Render\Renderer->doRender() (Line: 248)
Drupal\Core\Render\Renderer->render() (Line: 158)
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}() (Line: 638)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 153)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse() (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray()
call_user_func() (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch() (Line: 186)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 68)
Drupal\simple_oauth\HttpMiddleware\BasicAuthSwap->handle() (Line: 53)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 116)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 90)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 741)
Drupal\Core\DrupalKernel->handle() (Line: 19)

I guess the use case might differ for different projects. I for example had a use case where the client wanted to get a notification when a node was created, but not yet published (workflow review process).

I think it makes more sense to add an Access check to only show the notification if you have permission to view that specific entity. Would that cover your use case as well?

Thanks for testing. Merged and will tag a new release

Thanks!

@keshavv this ticket is closed for almost a year. It's recommended to create a new ticket and reference to this issue. Also note that the patch workflow is deprecated, you should use merge requests.

Assigning credits.

Thanks for looking into this and providing support!

Updated link and title structure

poker10 → credited bramdriesen → .

That commit only updated the info.yml, so not sure where things went wrong :-)

yet they are maintained by individuals who are part of the security review or advisory process.

What exactly is this referring to? People with the vetted role so they can opt-in a module? Let's use correct terminology :)

See #34 and #39

To propose improvements, you would need to accurately describe the current processes and how what the changes would be.

That has not been done in this issue, only misrepresentation. Categorising the responses you've received as a lack of 'openness' is further misrepresentation of the issue.

Those need to be in the issue summary (hence the tag "Needs issue summary update").

i don't neither understand the current status of the issue what mean, what are the needed information from which mantainer?

The status reads: Maintainer needs more info.

Meaning, the moderators need more info from you, the reporter. As we're just circling around about what the goal here really is.

esmoves → credited bramdriesen → .

Can you create an issue fork and merge request?

I also don't think we need such a "complex" IF-ELSE structure.

Thanks!

I guess we can add a type checking to see if we have a link/url object.

I think you forgot to upload the image. Also looks like the wrong type of tab indenting was used (see eslint report).

error Replace `↹↹↹↹↹` with `··········`

@bigbabert I think you're misinterpreting a lot of things being said here. The fact if a junior can or can't write code with or without bugs really doesn't matter here. The real issue is that people are just blindly "accepting" what those LLM's generate and push potentially dangerous code if they don't take the time to properly review what was generated.

I'm using CoPilot in PhpStorm since it's release. And I must say, what it spits out sometimes really makes my eyes frown. It's a really handy tool to have, and does save a lot of time, if used properly. I'm only using it to write single lines of code, more like a "autocomplete" like you are suggesting. For things that I used to google each time (such as the date function in PHP, some specific entity/database query, or even just translating .po files) it really saves time. But when it tries to do larger things, I always skip it since the output is still not good enough to be used. Even the most simple design principals are skipped, resulting in poorly written and none performant code. Fixing and reviewing all those things are simply not worth it, and it's a lot faster if I just write it myself, properly from the first time (pretty much what Cmlara is saying in #40).

Again the thing to take away here is: AI works great, if used and monitored properly. And I don't think many people have issues with using AI like that. The push back in this issue is about entirely generated modules, without proper knowledge/review of what it's doing. Yes what it spits out might work when you tested it, but is it safe? Performant? Following Drupal's or even just PHP best practices (not talking about PHPCS et al)?

Hmm not as trivial.

Drupal\Component\Plugin\Exception\ContextException: The 'entity:node' context is required and not present. in Drupal\Core\Plugin\Context\Context->getContextValue() (line 73 of core/lib/Drupal/Core/Plugin/Context/Context.php).

bramdriesen → created an issue.

Can we create a merge request for this?

Looking at the code removed in the setSessionInfo function, the php code doc should also be changed?

Re #7, probably not, but that was the default value when creating a core security issue from the security team docs ;-).

Adding a related issue which was mentioned in the private issue queue. However to me that is not the issue here but a broader discussion.