🇧🇪Belgium @borisson_

Locally this works now. I would like it if someone else could verify this.

I think we should also remove the maintainer and supporting organisation from the file.

borisson_ → created an issue.

it's a sub page of https://www.drupal.org/docs/develop/standards/sql → , wich is sql coding standards, but that is only an overview page. Should the coding conventions page actually be coding standards?

I updated the text and added myself as supporter in the new template.
We should find a sniff that is stricter than the on in#15, for reasons in #16.

Discussed in the office with Glenn, so crediting as well.

borisson_ → created an issue. See original summary → .

Merged

################
# GitLabCI template for Drupal projects.
#
# This template is designed to give any Contrib maintainer everything they need to test, without requiring modification.
# It is also designed to keep up to date with Core Development automatically through the use of include files that can be centrally maintained.
# As long as you include the project, ref and three files below, any future updates added by the Drupal Association will be used in your
# pipelines automatically. However, you can modify this template if you have additional needs for your project.
# The full documentation is on https://project.pages.drupalcode.org/gitlab_templates/
################

# For information on alternative values for 'ref' see https://project.pages.drupalcode.org/gitlab_templates/info/templates-version/
# To test a Drupal 7 project, change the first include filename from .main.yml to .main-d7.yml
include:
  - project: $_GITLAB_TEMPLATES_REPO
    ref: $_GITLAB_TEMPLATES_REF
    file:
      - '/includes/include.drupalci.main.yml'
      - '/includes/include.drupalci.variables.yml'
      - '/includes/include.drupalci.workflows.yml'
#
################
# Pipeline configuration variables are defined with default values and descriptions in the file
# https://git.drupalcode.org/project/gitlab_templates/-/blob/main/includes/include.drupalci.variables.yml
# Uncomment the lines below if you want to override any of the variables. The following is just an example.
################
# variables:
#   SKIP_ESLINT: '1'
   OPT_IN_TEST_NEXT_MAJOR: '1'
#   _CURL_TEMPLATES_REF: 'main'

borisson_ → created an issue.

Created 📌 Investigate if we need to have drupal/core as an explicit dependency Active as a followup

Asked the question in Drupal Slack's contribute channel:
https://drupal.slack.com/archives/C1BMUQ9U6/p1739367786307179

borisson_ → created an issue.

Actually, looking at this again it looks like we shouldn't really need to have drupal/core as a dependency, let's figure that out in a new issue.

This one no longer applies, but it looks like we need it in our project as well. Back to needs work.

This is not needed just for drupal 10, but it is for monolog 3 module?

Merged the change to composer.json

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

Applied #4, thanks everyone!

I agree, the test failures here do not look related. The change looks good and since it's a widening of the type, I think we don't need a change record.

Filled in the when TBA,
return static is only useful because it can do late binding, which is really helpful when that class is extended by another one. However final classes cannot be extended, so they can use self instead.

Good question, I think we do.

I think I would rather disallow type-hinting with \stdClass at all to resolve this, then it not being allowed in the PhpDoc would not be a problem.

I agree, most places where you would need a stdClass to be typehinted should ideally probably be value objects instead, so I think this moves us away from that.

Looking at @drunken monkey's comment in #3, can we close this as fixed?

I think the standard should still exist, and the usecase in the IS should nowadays be an enum, which already doesn't require comments on each property.

I think we can close this as won't fix, given the lack of interest on this issue in the past 11 years.

We are running into this issue as well. If I were to get some time to work on this, where would I start?

This is reducing type accuracy as mentioned by @mstrelan, but mixing inheritdoc and documentation is not allowed. What's the best way forward here?

Looks great, the phpcs.xml change ensure we don't regress here.

I looked at the entire MR again, since it came up in one of Wim's XB weeks: https://wimleers.com/xb-week-24
I think the merge request looks great, and I don't have any big open questions, it needs a rebase and Wim needs to close some of the open questions (that's still only possible to do for the mr author).

We also need to find an answer to the question highlighted in #282.

Since this was added to the IS, there was a complete rewrite of the patch, is it still valid? Do we have to postpone this on that?

Needs further investigation: #2905594: Missing entity validation constraint: don't allow new entities when there is an existing one with the same ID by @dawehner in #181. This might be a bit of missing test coverage?

Since we know that there will be a followup for the ruleset, this is a good start getting us to comply to the rule. +1

The changes here look good, but since there is no rule in phpcs.xml, how can we ensure we don't regress?

The rule is now enabled in phpcs.xml.dist, all places that were not yet compliant are now fixed. Looks good to go.

This is a large set to review, but they all are very similar. This looks good and the pipelines agree.

#18

From my personal experience, views with tons of exposed filters are usually admin views (for which I couldn't care less about the URL), and for user facing views with lots of filters I'll always try to use Facets which doesn't have this problem. ¯\_(ツ)_/¯

This is now different with facets 3.x, which can now use views exposed filters instead of its own url handling, rendering and ajax handling.
This means we now frequently get this question as well.

Reviewed the documentation page, that still had visibile html tags, fixed those. I think this is done now.

Fix the code examples, as the <code> tags were visibile still

How is this intended to run? Inside packaging of a deployment artifact for end users? Then it could just be document for folks on how to minimize container images and other artifacts.

I think "just documenting" is not good enough, having good defaults is a thing we try to do everywhere in Drupal, so it should be the same for our sustainability efforts.

I think we could add this to core-recommended for example, since that is what sites use, and not what you'd use when trying to develop for drupal?

A much more aggressive solution could be https://github.com/dg/composer-cleaner. I tried this on a laravel project recently and that saved another ~50mb.

I agree with catch, this is not simply for frontend usage. However it might be useful to help figure out why some entity is not indexed into search api correctly.
It doesn't look like there's an easy way for search api to reenable this for those purposes?

I agree with @longwave, this now gives an empty title and that sounds fine to me. I think we can close this issue.

bramdriesen → credited borisson_ → .

I don't have any remarks. I think this looks good. It does make it so that coder has to be a bit more careful when adding new rules I guess?

Looks good to me now. Thanks!

I think we can do better making this more consistent with the existing comments.

When working on this, we have to find a way to do this without hardcoding the list of proxy files? Is there a way to gather all the files that are proxied? Perhaps we should introduce a marker interface?

The latest run on the pull request doesn't succeed. I should be rebased. The follow-up in #24 is what resolves the questions in #21.

So I think this needs a rebase (and possibly a rerun of the script in #24), then it is good to go imho.

Created 📌 Fix Drupal.Commenting.FunctionComment.MissingReturnComment in core/lib/Drupal/Core & Components Active

borisson_ → created an issue.

Changes look good, phpcs.xml.dist has been updated and the pipelines are green.

Looks good, pipelines are green

The issue doesn't completely fix the issue as described in the comment. It talks about only excluding lib/Drupal/Core, however it also excludes lib/Drupal/Component

    <exclude-pattern>core/lib/Drupal/Core/*</exclude-pattern>
    <exclude-pattern>core/lib/Drupal/Component/*</exclude-pattern>

I think we should commit this as-is and make the followups also fix Component. So I think this looks good.

I had one remark, it is however an existing pattern, so no need to change in this instance. Marking as rtbc.

Marking as rtbc, the pull requests removes the rule mentioned in the IS.

With perhaps the added thought that we already moved from hooks to events, so moving back might be a bit annoying to anyone who already converted their hook implementations.

I think this is a big argument to keep moving ahead in the direction we already established.

I think you closed the merge request, but yes, I think we should.

Pipelines are green again after the rebase, looks good to me.

#24 sounds very reasonable.

This enables a sniff that is already passing, so this looks good to go to me.

Looks good.

This is great, I love that we're able to remove so many calls to t.

Everything that was in the queues at the time of 24hrs after the meeting is in this issue.

+1 to allowing optional comments.
+1 to allowing those comments to be formatted as a formal docblock
+ 1 to allowing optional new lines where appropriate for legibility.
-1 to requiring comments.
+1 to requiring comments to be formal "docblock" style.
-1 to requiring newlines.

This branch needs a rebase, there is a merge conflict in core/modules/inline_form_errors/inline_form_errors.module in the merge request (!9083).

I think #98, #99 and #100 are only partly resolved, there's still naming inconsistency, but I'm not sure what the better name would be. The documentation @alexpott asks for should probably go in a followup?

This needs to be changed to use a branch instead of just a patch.

Looking at the implementation, I'm not sure we can add the config factory at that place in the argument list, since it will break implementations that have the other arguments already? It is a required argument, so maybe it is in the right place. I'm not sure about this.

We're using this patch as well, seems to work flawlessly.

We've been using this patch for a while. I think it looks good to go, but not sure about tests? I'm going to rtbc this - but I'm not sure about the test-expectations for this.

This works for us as well, I'll give it another RTBC++

I agree that for seasoned Drupal developers these docblocks are redundant, but it still makes sense for junior developers, or developers coming from other platforms to provide context. I can imagine a Symfony developer that is doing a Drupal 8 project being confused by these weird ".module" and ".install" file extensions. It are exactly these files that contain our odd hook-based drupalisms, and a single line of documentation goes a long way to inform people what this is all about.

With the current way core seems to be going in trying to remove most of these files, by making hooks annotation based, it looks like the argument is start to lose value?

I think this is working as designed.

I agree, will comment on the other issue.

quietone → credited borisson_ → .

Added as supporting.

You may omit the description if using @return $this or @return static.

This is already in the docs. So that should be fine already? What do we need to do for this issue?

The suggestion was that our docs should point to PHPUnit docs. I currently disagree with that because of the '@group'. There is no documentation to explain how that is used in Drupal.

I agree we can't just point to the phpunit docs, but I think we should link to them + explain our own additional rules about them if they exist.

So, the question here is whether we need a @file doc block for this class file. If so, what should it say? If not, we need a new/revised Coder rule to detect/fix/remove this doc block.

I don't think I've ever seen documentation that lives in @file that is useful. I think we need the coder rule to remove this docblock from all files, including test files.

Setting to active, this is now ready to work on.

I'm not sure how strict maintainers are about tests, but I'm not sure this needs them, it's a very simple fix to allow that a custom auth provider can be used.

I updated the IS to be more complete.

I think we should also convert the patch to a merge request, setting to NW because of that.

esmoves → credited borisson_ → .

yes

I think this looks good now, there seems to be a failure in CronRunTest, is this a known random fail?

I think the scope of the fix is too wide. I think the changes in the tests and to the constructor are not needed?

I think only AdminNegotiator::applies and the changes to StandardPerformanceTest should stay.

I'm not sure about this though, so @sayco don't change this yet until we get confirmation what the right path forward is.

Added a very basic line of what we need to add. Should be refined.

#22, this is about calls to functions specifically, because of the performance part. Yours is different enough to warrant it's own issue. Especially since I think it is a lot less controversial.

bramdriesen → credited borisson_ → .

I had a question on the MR, but I don't feel strongly enough about it to un-rtbc this.

I agree with #4, looks interesting, but definitly needs tests.

I think this change on it's own is a good thing already, and followups can be created to make changes in AdminNegotiator?

@peter törnstrand https://www.drupal.org/node/3442349 →

Directly moving this to rtbc, since it's not only widens visibility, so it cannot be considered an API break, nor can it break other implementations.

I agree, making this private breaks the possibility for us to extend this class, if it is not intended to be extended, it should probably be final so it's not even possible.

I agree that the documentation is not up to date as-is and we should update it to be clearer, was just looking into how to set this up and we could only find the link to the drupal 7 documentation, with a big banner saying that drupal 7 is no longer maintained.

Can we make sure the drupal 8 documentation is on drupal.org, or place a prominent link on the module homepage to the readme.txt?

White this is really difficult to figure out how to trigger this without using any contrib. I seem to have the same issue when running database updates with drush when using a subtheme of the bootstrap theme. This breaks hard and gives an error I don't know how to pass.

The patch by @jungle works great to work around this issue and seems like a simple hardening of the code.

I updated steps to reproduce, but I'm not sure if they are enough or simpler to do. This is for a project where I didn't do the setup myself.

@smustgrave, you might know how we should proceed here? Using a contrib theme is the easiest way to reproduce the issue.

Thanks @mlncn!

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

Should this be considered a BC break, or can we commit this without a problem?