Hello all, it’s time for the fortnightly coding standards meeting.

This meeting:
➤ Is for anyone interested in the Drupal coding standards.
➤ Is held on the #coding standards channel in Drupal Slack (see www.drupal.org/slack for information).
➤ Usually happens fortnightly. Alternating between Tuesday 2100 UTC and Wednesday 0900 UTC.
➤ The meeting open for 24 hours to allow for all time zones.
➤ Discussion is done in threads, which you can follow to be notified of new replies even if you don’t comment in the thread. You may also join the meeting later and participate asynchronously.
➤ Has a public agenda anyone by adding a comment to the meeting issue.
➤ A transcript will be made using drupal-meeting-parser and posted to the agenda issue. For anonymous comments, start with a :bust_in_silhouette: emoji. To take a comment or thread off the record, start with a :no_entry_sign: emoji.
➤ The transcript will include comments made during the 24 hours of the meeting. However, comments made after the 24 hours may not be in transcript.

Current ping list: @catch, @larowlan, @longwave, @quietone
@dww, @borisson_ @longwave @Björn Brala, @Aaron McHale, @Alex Skrypnyk, @Urvashi, @Kingdutch

Comments

quietone created an issue. See original summary.

larowlan credited bbrala.

larowlan credited borisson_.

larowlan credited kim.pepper.

larowlan credited Kingdutch.

larowlan credited longwave.

larowlan credited mstrelan.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

0️⃣ Who is here today? Comment in the thread to introduce yourself. We’ll keep the meeting open for 24 hours to allow for all time zones.

quietone Good morning. Another day of rain here.
longwave :wave:
kimb0 :wave::skin-tone-2:
mstrelan :wave:
borisson_ :wave:
Björn Brala (bbrala) Hi there

1️⃣ What topics do you want to discuss? Post in this thread and we’ll open threads for them as appropriate

2️⃣ Action items

2️⃣.1️⃣ Approve minutes

larowlan https://www.drupal.org/project/coding_standards/issues/3456131

2️⃣.2️⃣ TBD

3️⃣ Fixed since last meeting

4️⃣ RTBC issues

4️⃣.1️⃣#3339746: Coding style for PHP Enumerations

mstrelan There are some sniffs for enum spacing but not naming.https://github.com/slevomat/coding-standard/blob/master/doc/classes.md#s...
quietone Nice.
quietone Who wants to test those.
quietone And then I guess we need an issue in coder for case of the name?
mstrelan Yes, ideally it should be contributed upstream. There is an open issue at https://github.com/slevomat/coding-standard/issues/1649

4️⃣.2️⃣#3324368: Update CSS coding standards to include PostCSS and Drupal 10

larowlan At some point do we postpone this - if there's no interest from the original proponents in finalising the docs?
quietone Sadly, I think so.
quietone I do think the issue summary should include why it is postponed.

5️⃣ New issues

5️⃣.1️⃣#3458314: Convention or recommendation for line breaks in constructor parameter signature with promoted properties

quietone In principle this makes sense to me.
quietone Although, the Function section states that parameters are on the next line. https://www.drupal.org/docs/develop/standards/php/php-coding-standards#f...
quietone Maybe we need to combine that somehow?
longwave i think the proposed text needs a bit of work here to clarify the either/or options - parameters must all be on one line, or they must all be on separate lines
longwave but in general +1 for this proposal especially given we are already using it and it makes attributes and property promotion much more readable
longwave Parameter attributes can be placed on the same line as the parameter, or on a separate line.i think we should just say they should be on separate lines if you are using the multiline format already (edited)
mstrelan See also https://github.com/slevomat/coding-standard/issues/1684

6️⃣ Active issues

6️⃣.1️⃣#3263602: Allow type hinting with classes where appropriate

quietone This has three proposed text changes one of which is to the same section that is being changed in[#1158720]
quietone This issue needs examples for when to use an interface as a type hint and when to use a class.
quietone Anyone able to add those?
Kingdutch I'm hard pressed to come up with them within Drupal. So I'm mostly curious what prompted the creation of the issue 2 years ago :smile:In our own contrib code I've definitely seen interfaces that don't make sense, but that's more because the class that it was written for isn't focused enough or shouldn't be a class but a few functions. Not necessarily because interfaces are bad.It could be that it's a bit "concept" vs "value"? :thinking_face:For example for services and entities we'd always want interfaces because they may be decorated and we really only care about the concept/contract.However if you have a class like the Result class I'm trying to introduce elsewhere, an interface doesn't make sense because there's no other implementation of Result that would also represent the same concept.
borisson_ I think that is a good example, AccessResults are another good example of not really needing an interface.
Kingdutch I think that is a good example, AccessResults are another good example of not really needing an interface.Interesting example, because I do think the interface is useful. For some reason you may want to have an AlwaysAllowed that overwrites the andIf function to return itself. In that case you don't need the static methods that AccessResult provides (because they do something else than you expect from AlwaysAllowed). However, I can also see the entirety of Drupal failing if you give it AlwaysAllowed and it doesn't implement RefinableCacheableDependencyInterface

6️⃣.2️⃣#1368794: Using '&' in doxygen @param documentation

quietone I can't say that I support this one.
Björn Brala (bbrala) Me neither to be honest. Mostly a lot of work which doesn't seen to add that much to better understanding the code.
Kingdutch I'm on the fence. Overall I think the docblock should reflect what's happening and it's a bit silly that this isn't in there.Having said that, I expect that pass by reference becomes less and less common (because you really shouldn't change arguments you get) and we're moving to other type-hints which are generally not passed as reference.Additionally I see that with PHPs improving type support the types in docblock matter less and less (except where they're more specific PHPStan/Psalm types that PHP doesn't support yet such as non-empty-list)

6️⃣.3️⃣#1539738: list() syntax

quietone My last comment show some testing with the current sniffs in core. There was no detection of the "list()" syntax.

6️⃣.4️#3280642: Use fully-qualified class names in code in api.php files

Björn Brala (bbrala) Hmm, basically that would mean an addition to the documentation where we talk about FQCN's. But that might mean we need to check the other way around (all being FQCN's), unless we make it MAY/SHOULD, but that then kinda waters down the the result.I'd be for documenting behaviour, but rather be explicit i think
Kingdutch Is there instead a discussion possible where we make .api.php the same as all other code and allow use statements? Especailly since if I randomly open such a file (viiews.api.php) it already has some use-statements and is thus mixed.In general I think code in .api.php should be as close to normal code (thus ideally also working and properly typed so PHPStan-Drupal doesn't choke on it) as possible.

6️⃣.5️⃣#3259666: Require backslash prefix for global functions

quietone I reduced the scope on this last night and worked on the core issue that implements the sniff.
quietone The sniff fails in one case where there is an sprintf in an @trigger_error message.
mstrelan I've seen lots of people reference the benchmarks at https://veewee.github.io/blog/optimizing-php-performance-by-fq-function-... when discussing this, but noticed that the benchmarks are for php 7.1, so I decided to do my own benchmarks on both 7.1 and 8.3, with and without opcache.tl;dr without opcache its all the same, but with opcache calling the global function without the backslash is about 1.5x slower
mstrelan php 7.1 - 0.023 μs with backslash and 0.036 μs withoutphp 8.3 - 0.014 μs with backslash and 0.024 μs without
Björn Brala (bbrala) If that with 100 iterations of 1000 tests?
Björn Brala (bbrala) The increase in time is pretty significant it seems. Would be interesting to know if changing this in core actually has an effect on core performance tests.
mstrelan It's with exactly the steps in the blog post
mstrelan We're talking fractions of a microsecond
borisson_ Yeah, not significant at all, and especially for booleans it's also very ugly: (the return \true really doesn't look right to me).But I really think we should do this, it's only fractions of a microsecond for one drupal install, but the combined energy impact over all the drupal installs that run everywhere is probably worth it. Especially since it would be very easy to introduce
mstrelan True, although failed ci runs when people aren't auto formatting this would be significant too
borisson_ Good point. I guess it will be less still, but I didn't consider that.
Björn Brala (bbrala) Oh fractions of ms
Björn Brala (bbrala) Yeah that is useless. I was reading ms
Björn Brala (bbrala) Energy impact I'm not too sure about. It could technically reduce bugs. Although I wonder if namespaced overriding function are even really used.

6️⃣.6️⃣#3422530: Require short ternary (Elvis operator) syntax

quietone This will need a sniff
Björn Brala (bbrala) The sniff is in the related issue it seems?#3422531: Enable SlevomatCodingStandard.ControlStructures.RequireShortTernaryOperator
Björn Brala (bbrala) So basically, if we should be all set if we agree its a fine idea.
Björn Brala (bbrala) I would like to add my +1 for this.
borisson_ I think we need to update this issue to the new template, when that is done we can add +1 for those supporting the change.
borisson_ Oh no, I clicked the wrong link. It is already in the new template
borisson_ In that case, I think this is ready for the next step?

6️⃣.7️⃣#2325985: No documentation about @group @coversDefaultClass @covers

mstrelan These are all deprecated in phpunit 10. There are replacements that will be required in 11.
quietone Oh, nice to know.
quietone Can you add a link to the replacements? (edited)
mstrelan Afk, but see[#3445900] and related issues and the link to phpunit website. Tl;dr we need to use attributes instead, and you can't mix annotations and attributes as phpunit will stop searching once it finds one or the other.
mstrelan IMHO any docs should just point to phpunit docs

7️⃣ Should we try to recruit a manager to help with admin tasks such as running meetings, blog posts etc. A lot of the people on the committee wear multiple hats (core committers, subsystem maintainers etc). These tasks take us away from other contributions

quietone Sure.
quietone I also think we could improve our own planning for these tasks. As in use our calendar.
Björn Brala (bbrala) Yeah i do agree, although i haven't been running these it would be good to have the load spread more.
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Active » Needs review
bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs review » Reviewed & tested by the community

Seems all good. Ty.

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Reviewed & tested by the community » Fixed

Closing, all seems good as mentioned earlier. No corrections since.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.