🇧🇪Belgium @borisson_

I missed this issue when it was originally active, we should start by rerolling this.
Next we should look at https://git.drupalcode.org/project/drupal/-/merge_requests/7962#note_333039, to figure out how the schema should look.

Huh? *No one* in the "allow whitespacing" camp is saying all grouped variables MUST be separated by columnar whitespace.

No that is true, but there are people (like me), that cannot stand the "allowing whitespacing" style, and there are people that find the "only 1 whitespace" allowed style to be a lot less readable. Maybe I am misunderstanding, English is not my first language.

It looks like we have found an issue that people are very much set in their ways about.
This is however something that we really need to find a rule for, because having both styles mixed in a codebase is not great in my opinion.

Is there a way we can get a poll out to the wider community to vote on this issue, we normally try to build consensus around coding standards issues, but I think this one might be difficult to find consensus on.

borisson_ → created an issue. See original summary → .

Credit from 🐛 Compatibility with 'Itspire - Monolog Loki' ^2.0 Needs work

Fixed in 🐛 Drupal 10 compatibility Active

Merged.

Manually tested this in the office with @ghuygens. This works. I'm going to commit this.

The only place where the rule decreases readability in this mr is the one alex pointed out.
In the other places it doesn't change readability in a negative way. I am not sure what the best way forward is here, should we change the rule to allow more characters or is there another way we can make it easier?

Changed the wording.

I agree that there's not been consensus yet.

I don't think CommonMark allows for having tables, and I think that is an important extension to have. It is in Github and Gitlab flavored markdown, and given our integration with Gitlab, it might be a good idea to go for that?

Using the header and vary on that is very dangerous for cache purposes. It will make very very many variations, which results in bad caching and performance. Not to mention possible memroy issues in caches or high cache trashing.

While this is true for a web-based environment, it is not true for JSON:API.
Quoting @GabeSullice from https://www.drupal.org/project/drupal/issues/2794431#comment-12806367 🌱 [META] Formalize translations support Needs work

However, JSON API doesn't need to be very concerned about that because JSON API requests are not typically made directly via a browser. Instead, they're most frequently made via JavaScript or some other system where the Accept-Language header can be tightly controlled by a developer.

Looks great, thanks!

I think this is ready, the rule is enabled and the remaining issues are fixed.

Well, system.action.* is fully validatable, but you are right that the plugins aren't.
Depending on the metric you use, all the config in core is at around 42% validatability. https://project.pages.drupalcode.org/config_inspector/

So, if we would have to keep all the plugins in mind as well, I think we would probably have to remove even more of the checkmarks from this list. I don't think we have open issues for all the plugins, but I guess we can create those issues to make the plugin validatable again.

So we clearly aren't there yet, but I guess we can look at the individual rules that make up that phpcs --standard=PSR1, and introduce them into our own phpcs.xml, so that we can eventually say we are compliant with PSR-1?

You are right that is confusing to first time users of this module that we do the dynamic swapping of the service, I don't know the reasons for doing it this way, we could look into changing this, but it would lead to a significant BC break, so it would have to be done in a new major release of this module.
I'm not sure if this is worth it.

Increasing this is not a good idea because of the mentioned impact on performance. I think we should keep these defaults as-is because they keep people from creating very slow websites.

Readin the discussion there, it is suggested to keep both, however it would be good to add a test to keep them in sync.

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.