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’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Additional agenda item - should we drop keys for argument names in data-provider test-cases - #3442167: Fix string array keys in data sets returned by data provider methods that do not match the parameter names in Kernel tests

mondrake’s picture

mondrake
Location 🇮🇹
commented

#2 an argument in the opposite direction ie add keys where they are missing is in #3443535: Change @dataprovider to static in SqlContentEntityStorageSchemaTest

bbrala’s picture

bbrala
Location Netherlands
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.

urvashi_vora Hello :wave:
catch :wave:
longwave :wave:
Björn Brala (bbrala) :mario-wave:
Kingdutch :wave: In between meetings
larowlan hi

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

2️⃣ Action items

quietone complete minutes for Coding Standards Meeting Wednesday 2024-03-13 0900 UTC. Coding Standards Meeting Wednesday 2024-03-13 0900 UTC
quietone This meeting did not happen.

2️⃣.1️⃣ Approve minutes for previous meeting(s)

#3440953: Coding Standards Meeting Tuesday 2024-04-23 2100 UTC

urvashi_vora Looks good to me, verified and Fixed.

2️⃣.2️⃣ TBD

quietone @quietone: follow up with the CSS standards
quietone Publish the blog post

3️⃣ Fixed since last meeting

quietone #3098745: Should exit(); and die(); be called with or without parentheses?

4️⃣ RTBC issues

4️⃣.1️⃣  #3295249: Allow multi-line function declarations (edited) 

quietone The IS shows all steps complete. What is left to do?
quietone A change record update to include the point in comment #65 would be helpful.
quietone Other than that I think this can be set to Fixed.
quietone I'll update the CR now.
quietone I just read the comment properly and it is for unsupported versions of Drupal so maybe best to leave it in a comment?
Björn Brala (bbrala) Yeah keep it there.
Björn Brala (bbrala) 7.4 is not supported also, and should you run the rules on 7.4 it will not add the comma (i looked at the code earlier, it is versino aware)
quietone Ah, that is nice to know.
Björn Brala (bbrala) The core issue needs rebase/fix (phpcbf).
Björn Brala (bbrala) https://git.drupalcode.org/project/drupal/-/merge_requests/7693#note_304... issue was introduced it seems.Major - Multi-line function declarations must have a trailing comma after the last parameter

in /builds/issue/drupal-3443117/core/modules/navigation/src/Form/SettingsForm.php:77

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

quietone Step 5. To be discussed
quietone This is tagged for final discussion so it needs a blog post., I think.
quietone I don't see a blog post.
quietone Added a comment for this on the blog issue and updated the title with a date. I hope that will help us remember to make the blog post #3443092: [2024-05-15] Prepare blogpost for annoucement/updates
quietone That issue has a link to a google doc for the post. So anyone here can help work on it.
quietone I updated the blog post for this. Can someone proof read it?
larowlan blog post looks good, do we have anything else to add?
quietone I had a thought late last night that we should let people know that we are using the change records to announce changes to the standards. And do that as well on the project page.

4️⃣.3️⃣ #3074131: Use null coalescing operator ?? instead of a ternary operator with an isset() condition (edited) 

quietone Step 8. Doc updates
quietone Ah, bbrala did the doc updates. So, someone needs to confirm they are correct.
quietone And it needs a core issue, unless it has been made.
Björn Brala (bbrala) I actually did the core conversion on 2021 - #3222251: [November 8, 2021] Replace all isset constructs with the null coalescing operator
quietone Nice.
quietone I have been reviewing this and all I see is that the CR needs to be published. There is a core issue to enable to sniff.
Björn Brala (bbrala) awesome
Björn Brala (bbrala) Shouldve asked before also checking hehe
Björn Brala (bbrala) Think you misread, there was no core issue for this one specifically.But ive made one, updated IS, and pushed the fixes to the related issue xD[#3446107]
Björn Brala (bbrala) Published CR and transitioned to fixed.
Björn Brala (bbrala) Guess contribution day is happening, the child issue was committed to 11 and will be backported to 10 in a minute is seems o_O

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

quietone Step 8. Doc updates.
quietone I still need to reach out to @mherchel and make progress here.

5️⃣ New Issues

5️⃣.1️⃣ #3444003: The text of an @todo should start with a capitalized word and sniff non-php files

quietone Related to core issue that I have been working on
quietone #3180696: Fix 'Drupal.Commenting.TodoComment' coding standard

5️⃣.2️⃣ TBD

6️⃣ Old Issues

6️⃣.1️⃣ #3117816: PHP 7 Group Import Declarations

quietone The two comments don't support this change.
quietone I also think it makes the use statement harder to scan
Björn Brala (bbrala) Agree, we should not do this.
Björn Brala (bbrala) I'll comment, and wait for more comments and close tomorrow if no others disagree
borisson_ I agree, we should not allow this.
Björn Brala (bbrala) closed

6️⃣.2️⃣ #2304937: Please remove this requirement: 'WARNING | Interface names should always have the suffix "Interface"'

quietone I read this many months ago. It doesn't have support.
acbramley Added my support against this.
Björn Brala (bbrala) i concur also
Björn Brala (bbrala) with being against it lol
borisson_ I actually really like the way it's done a lot in the laravel world, where they are a bit more discriptive, for example when you want a job that is on a queue, you implements ShouldQueue, (not QueueInterface). Or when you want a model to have media you implements HasMedia, I think that reads very nice in the class you are looking at, however this is a lot harder to have in a rule
Kingdutch I think in general his arguments about OOP naming (and your example of Laraval) are actually quite good and you see it in traits of other languages. In our own code I'd probably write your example QueueableInterface or WithMediaInterface which has the suffix that's standardised for us but als the "intention" naming.However, that breaks down/becomes difficult when you move from the classic OOP examples (e.g. "animal" interface). I think especially in Drupal there's a lot of interfaces that don't indicate a concept, but instead indicate a contract for a subsystem, with the goal that you can replace entire Drupal subsystems.For example (with a lot of effort) I could write an entirely alternative entity system by implementing all the related interfaces. Whether that's IEntityTypeManager or EntityTypeManagerInterface is honestly a toss-up. Java landed on the former, PHP landed on the latter :man-shrugging:  But something doesn't really behave like the EntityTypeManagerInterface, it implements the contract for that subsystem.My big argument against IEntityTypeManager is actually that the interface is now suddenly not right next to the default implementation in my file browser, or in any list of classes. I'm realising this as I type. It would instead be somewhere in the middle of the list with a bunch of other likely unrelated interfaces.
borisson_ Laravel actually has a contracts directory, were only some of those have intention naming and others are more like the EntityTypeManagerInterface example
borisson_ I'm not saying I like that better, but having all interfaces in their own namespace largely removes the problem that interfaces and classes are mixed in the same directory and they are not right next to each other. They are actually a lot further away on the file system
Kingdutch I'll admit that looks enticing 🙂 I'm also firmly in the camp that Laraval has been able to learn from other projects and improve upon it which is awesome. However, I'm (likely with many) unwilling to accept living in the inconsistent in-between with Drupal/Symfony.Which I guess is slightly sad for the original poster of the issue, since they did mention they wanted to discuss this before we had a lot of Drupal 8 OOP. But in the end I'm also firmly in the camp that a clear consistent system (which both Drupal/Symfony and Laravel have) is more important than what that system looks like.So I'd still be against the proposal of the issue.
borisson_ I agree, consistency is super important. Because of that I also think we should not implement this, especially at this stage in the oopification of drupal.
Björn Brala (bbrala) Closed the issue.

7️⃣ Wrap up. Keep chatting in the threads and and news one. Meeting is open for 24 hours.

quietone Thanks for attending!
Kingdutch Thanks for hosting! :tada:

Participants:

urvashi_vora, catch, longwave, bbrala, Kingdutch, larowlan, quietone, borisson_, acbramley

bbrala credited acbramley.

bbrala credited borisson_.

bbrala credited catch.

bbrala credited Kingdutch.

bbrala credited longwave.

bbrala credited urvashi_vora.

bbrala’s picture

bbrala
Location Netherlands
commented
bbrala’s picture

bbrala
Location Netherlands
commented
bbrala’s picture

bbrala
Location Netherlands
commented
Status: Active » Needs review
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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