Coding Standards Meeting Wednesday 2024-04-10 0900 UTC [#3438058]

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

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.

Kingdutch	Current ping list: @catch, @longwave,@dww, @borisson_ , @Björn Brala (bbrala) @Aaron McHale, @Alex Skrypnyk,@urvashi_vora
larowlan	Hey
Alex Skrypnyk	Hi :wave:
quietone	HI
catch	Hello.
urvashi_vora	Hi
Björn Brala (bbrala)	ty @Kingdutch and hi :stuck_out_tongue_winking_eye:
kimb0	:hand:
dww	Derek, late to the party

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 for previous meeting(s)

Kingdutch	#3438057: Coding Standards Meeting Wednesday 2024-03-26 2100 UTC (already RTBC so maybe done?)
urvashi_vora	Yes, we can mark it as fixed
dww	Fixed

3️⃣ Fixed since last meeting

4️⃣ RTBC issues

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

larowlan	Still with me for docs updates
quietone	I wanted to finish this today but didn't get to it.
quietone	I don't have to do my scheduled school run tomorrow so it seems like I have some free time. So, instead of a walk in the morning I can do this 🙂
quietone	The docs updates were completed on 2024-03-06
quietone	Added a comment for testing I did and then made an issue in Coder.
quietone	How are we informed that the Coder issue is fixed?
dww	I guess we just follow the issue and hope for the best? :sweat_smile:

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

larowlan	We added another enum to core with uppercase this week, the horse may have bolted
catch	We could still change those if we do so before 10.3 comes out, it'll be harder later.
catch	we might want to short-circuit the process slightly an open core issues for the existing enums to match the propose standard?
Kingdutch	We have the following steps under remaining tasks:Add supporters -- Can be found in comments or maybe this thread? Create a Change Record -- Something I could look at today Review by the Coding Standards Committee -- Probably blocked by CR? Coding Standards Committee takes action as required Discussed by the Core Committer Committee, if it impacts Drupal Core -- Some known issues to fix before 10.3.x Final review by Coding Standards Committee Documentation updates -- Is proposed and has a targeted fix (needs to be executed) Edit all pages Publish change record Remove 'Needs documentation edits' tag If applicable, create follow-up issues for PHPCS rules/sniffs changesI'd be slightly disappointed if we deviate from the rest of the PHP community for the rest of time because we weren't fast enough with the process :disappointed:
quietone	So, this one needs to be a priority. For example, there are no supporters.
quietone	I would like a simpler example as well. For me, the current example is too busy.
quietone	Anyone know if there is anything in Coder for this? (edited)
catch	added myself as a supporter.
Kingdutch	So, this one needs to be a priority. For example, there are no supporters.I see the following people with +1 for UpperCamel in the comments:gabesullicemondrakeFatherShawnfenstratThe Symfony framework (probably doesn't count :stuck_out_tongue: )dale42KingdutchacbramleyNot sure if that's explicit enough or the supporters need to be explicitly coding standards team members? :smile:
Kingdutch	I would like a simpler example as well. For me, the current example is too busy.@quietone Would removing the returnEarly function fix that for you? It shows how an enum would look but removes the case-usage example
catch	I think we can document it and update core without the coder being sorted out, we already committed stuff to core with no standard - which is fine for new PHP features it's just unfortunate that we acciddentally created a standard.
borisson_	Added myself as a supporter as well
quietone	Re Coder. I just want to make sure that there is an issue over there for this.
quietone	For the example, it is the function itself. I think it all the wrapping of lines which just makes it harder. I lean towards examples being simple.
catch	@Kingdutch if you add yourself as a supporter that's the three we need.
quietone	However, I am not going to block/slow this down by pushing that point. It can always be adjusted later.
catch	I would also drop the usage example, it's not part of the standard.
quietone	Ah, yes, just add "// do stuff"
Kingdutch	Done :smile:
Kingdutch	I think for the doc I should've moved the { onto the same line as enum right? To align with other Drupal coding standards on this.
Kingdutch	Created a proposal for a CR: https://www.drupal.org/node/3439920Would love a second opinion on the title.
quietone	There is work to do here. I left a comment on the issue.
quietone	Who wants to follow up with Coder on this one?
dww	I updated the CR, updated the proposed text in the summary, and updated the remaining tasks. Tagged as needing announcement, although it's not clear if we're going to take the time for that one in this case. :sweat_smile:
dww	I'm in favor of updating the existing enums immediately to comply with the new standard, even if it's not formally approved, since we're in sad shape if we ship 10.3.0 with them out of compliance.
dww	FWIW:% egrep -ir 'enum ' lib modules lib/Drupal/Core/Database/Transaction/StackItemType.php:enum StackItemType { lib/Drupal/Core/Database/Transaction/ClientConnectionTransactionState.php:enum ClientConnectionTransactionState { lib/Drupal/Core/Database/Event/StatementEvent.php:enum StatementEvent: string { lib/Drupal/Core/Form/ToConfig.php:enum ToConfig { lib/Drupal/Core/Theme/Component/SchemaCompatibilityChecker.php: 'Property "%s" does not allow some necessary enum values [%s]. These are supported in the original schema and should be supported in the new schema for compatibility.', lib/Drupal/Core/Theme/ExtensionType.php: * Enum for supported extension types. lib/Drupal/Core/Theme/ExtensionType.php: * @todo Replace this enum with Drupal\Core\Extension\ExtensionTypeInterface. lib/Drupal/Core/Theme/ExtensionType.php:enum ExtensionType: string { modules/datetime_range/src/DateTimeRangeConstantsInterface.php: * @todo Convert this to an enum in 11.0. modules/system/tests/modules/performance_test/src/Cache/CacheTagOperation.php:enum CacheTagOperation {Reviewing all those, I only see 2 that need fixing:modules/system/tests/modules/performance_test/src/Cache/CacheTagOperation.php (uses camelCase, not UpperCamelCase)lib/Drupal/Core/Database/Transaction/StackItemType.php (Savepoint -- should that be SavePoint ?)
larowlan	false alarm on https://www.drupal.org/node/3426517 looks like it complies
dww	#3440087: Update CacheTagOperation enum to use UpperCamelCase with MR
dww	(I'm super rushing while juggling other crap, so apologies if I botched anything so far) :sweat_smile:
Alex Skrypnyk	@dww[#3339746]#comment-15548396
dww	:table_tennis_paddle_and_ball: #3339746: Coding style for PHP Enumerations#comment-15548513 :laughing:

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

quietone	This looks ready for doc updates.
quietone	Anyone want to take on that task?
Björn Brala (bbrala)	I've set an reminder for next week.
quietone	And there is work to do about Coder on this one.
Björn Brala (bbrala)	Just remembered, think I did a 300kb patch for this for core ages ago. Ouite nly fitting if I do this :sweat_smile:

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

quietone

I wonder if someone here can help / coordinate with mherchel on this? Maybe that will help?

5️⃣ New issues

5️⃣ . 1️⃣ #3439004: Coding standard for attributes

5️⃣ . 2️⃣ New activity on needs review issue #1624564: Coding standards for "use" statements

dww	See additional info at https://drupal.slack.com/archives/C02LJCF78E8/p1712536767610059
dww	To me, this is (soft?) "blocked" on all y'all that use PHPStorm to figure out how to get your IDE to follow the standard. We've already got the phpcs rules sorted out (see the linked thread), but it's going to be hella disruptive if folks can't get PHPStorm to play ball.
Kingdutch	Added a link to the relevant PHPStorm issue in the Drupal issue: https://youtrack.jetbrains.com/issue/WI-26078/Automatically-added-use-st... PHPStorm users can upvote the issue we may get a fix, but it's been open for 9 years so I'm not hopeful :disappointed:

6️⃣ via @Jonathan1055 suggesting updates to the project pageItem for discussion, didn't want to raise a separate issue for it. Suggested update to the project page, see attached screen grab.Needs an line explaining what the links areCan the links be a in a larger font? they are major important parts of the project, but do not stand outThe one line of text about PHP could be moved to the linked page. It is odd that PHP has some extra words here, but the other sections/links do not. Better to have just clean links here, with the words on the linked pages.

Jonathan1055	Here's the screengrab
quietone	@Jonathan1055 This all makes sense to me and is an improvement.
quietone	I am going to make the changes now.
quietone	It is a little different than your suggestion but still better. At least, I hope so.
quietone	Why should the font size increase? They do stand out quite a bit.
dww	Yeah, now that the text for PHP is gone, I think the links are fine as-is...
dww	Thanks for raising this, @Jonathan1055 and for fixing it, @quietone. :pray:
Jonathan1055	Thanks @quietone for doing the update, it all looks good. Just one tiny nit-pikThe guide contains the detailed documentation for the following;It think it should be a colon at the end, not a semi-colon.
dww	Fixed
quietone	Nice. Thanks @dww for the great teamwork. 🙂

7️⃣ via @quietone #3099562: [Policy] Consider creating coding standards depended on specific PHP versions

quietone	Looking for opinions on this one.
quietone	I am thinking this is a won't fix.
catch	Yeah we have rolling PHP version requirements, as does PHP itself, won't fix is good.
borisson_	I agree, coding standards are a living thing, fixing them in time or per version of php seems like making it difficult. +1 to won't fix
Kingdutch	Agreed. Coding standards should be for supported features which should work across PHP versions. The only dependency we have on PHP version is for feature introduction (like with Enum) but in that case older versions can ignore the part of the standard because they're not using the feature.
dww	#3099562: [Policy] Consider creating coding standards depended on specific PHP versions#comment-15546712 -> won't fix'ed

8️⃣ via @quietone #2689243: Import classes used in annotations

quietone	I suggested this one because I think it can be closed as outdated.
quietone	Any other opinions?
Kingdutch	Ah I didn't really get it was also @annotation but thought it was e.g. also @param but I suppose that's a separate discussion?I think for annotations I'm happy to move this to "outdated" since we're mostly moving to attributes? It'd be interesting to start the conversation for other PHPDocs though.Found the other issue that I'm thinking of: #3360160: Stop using FQCN in PHPDoc annotations :smile:
Björn Brala (bbrala)	close as outdated, before we get this all up and running we might not even have annotatainos anymore.
dww	Closed: #2689243: Import classes used in annotations#comment-15546715

9️⃣ via me :smile:#3360160: Stop using FQCN in PHPDoc annotations

Kingdutch	I think since we're moving to typed properties in the code as well, the number of use statements this is going to add is relatively little so I think it might make sense to allow this.The added benefit is that as we start introducing more complex mixed types or generics their readability improves./** * @param \Drupal\module\Some\Name\Spaced\DataTypeInheritanceClass\|\Drupal\Core\Spaced\DataTypeClass $foo /vsuse Drupal\module\Some\Name\Spaced\DataTypeInheritanceClass; use Drupal\Core\Spaced\DataTypeClass; /* * @param DataTypeInheritanceClass\|DataTypeClass $foo */(edited)
quietone	This does seem sensible.
quietone	It should have the new issue template so we can keep track of progress.
quietone	Anyone up for adding it?
mstrelan	Particularly useful for generics
Kingdutch	Added the new issue template and added myself as supporter 🙂
borisson_	I'll add myself as a supporter as well
Björn Brala (bbrala)	oh god, please, this would be great.
dww	Oh hell yes! I've always wondered why we require FQCN for these docs.
dww	Started fleshing out the summary with the actual changes we need to make to the standards, but ran out of time, so tagged for needing a summary update:#3360160: Stop using FQCN in PHPDoc annotations#comment-15546723

Participants:

Kingdutch, larowlan, Alex Skrypnyk, quietone, catch, urvashi_vora, bbrala, kim.pepper, dww, borisson_, Jonathan1055, mstrelan

Comment	File	Size	Author
#2	project front page with suggestions.png	613.35 KB	jonathan1055

Comments

Comment #1

4 April 2024 at 07:36

larowlan created an issue. See original summary.

Comment #2

jonathan1055 commented 4 April 2024 at 08:05

Status	File	Size
new	project front page with suggestions.png	613.35 KB

Item for discussion, didn't want to raise a separate issue for it. Suggested update to the project page, see attached screen grab.

Needs an line explaining what the links are
Can the links be a in a larger font? they are major important parts of the project, but do not stand out
The one line of text about PHP could be moved to the linked page. It is odd that PHP has some extra words here, but the other sections/links do not. Better to have just clean links here, with the words on the linked pages.

Comment #3

quietone commented 8 April 2024 at 07:05

#3099562: [Policy] Consider creating coding standards depended on specific PHP versions
#2689243: Import classes used in annotations

Comment #4

jonathan1055 commented 11 April 2024 at 07:28

Thank you for making the changes from #2

Comment #5

quietone commented 15 April 2024 at 02:23

Issue summary:

View changes

Comment #6

jonathan1055 commented 15 April 2024 at 07:49

Thanks also for fixing the issue summary in #5. I saw that it was created with the default template. Is there an option for a project to have alternative templates? Or do you just keep this standard meeting text somewhere to copy/paste? Or do you take it from the previous meeting? Useful for others to know, to avoid work fixing things.