- ๐จ๐ฆCanada Charlie ChX Negyesi ๐Canada
Good first step but needs several followups
- Allow omitting doxygen altogether. It's just not necessary to add any doxygen to
protected LockbackendInterface $lockBackend
. Optionally yes but most of the time the type tells the story - Extend to @param and @return and entire method doxygen. Once again, these might be completely unnecessary.
protected function getLockbackend(): LockbackendInterface
, gee, I have no idea what this method could possibly be about, let me try to guess.
- Allow omitting doxygen altogether. It's just not necessary to add any doxygen to
- ๐ณ๐ฟNew Zealand quietone
Add the existing documentation to the Issue Summary.
It will help if someone would complete the proposed text section of the Issue Summary.
- ๐ฆ๐บAustralia larowlan ๐ฆ๐บ๐.au GMT+10
Took a shot at a proposed text
- ๐จ๐ฆCanada Charlie ChX Negyesi ๐Canada
Filed #3376518: Allow omitting doxygen when type and variable name is enough โ for a followup.
- ๐ณ๐ฟNew Zealand quietone
@larowlan, thanks.
I like examples, so I expanded on suggestion. - ๐ฌ๐งUnited Kingdom alexpott ๐ช๐บ๐
quietone โ credited alexpott โ .
- ๐ณ๐ฟNew Zealand quietone
Closed #3291517: [policy, no patch] Use type declarations for new class properties โ as a duplicate. I am adding credit.
- ๐ฏ๐ตJapan tyler36 Osaka
+1 Feels like we are embracing PHP conventions.
- ๐ณ๐ฑNetherlands bbrala Netherlands
This has been standard practice in our client code for ages. +1
- ๐บ๐ธUnited States dww
- s/type hint/strict type definition/?
- Should the new text say that strict types are preferred wherever possible?
- ๐ฎ๐นItaly mondrake ๐ฎ๐น
IMHO the examples should be more generic. The
$dbh
property is long gone from the Database API and regadless of that it was a very special case being a PDO related property. - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
๐ Yes, please! Far less boilerplate ๐
- ๐ง๐ฌBulgaria pfrenssen Sofia
Updated the wording taking into account the suggestions from #28 and #29. Switched from a public property to a protected one. Used the EntityTypeManagerInterface since this is very recognizable, and uses the interface as a type which is the best practice.
- ๐ฌ๐งUnited Kingdom catch
I agree with this as it stands, but it seems worth noting that ๐ Use PHP 8 constructor property promotion for existing code Needs work means that any property promoted via a contructor won't be defined separately at all, so the entire statement goes in that case. That makes me wonder how many strictly typed properties will be left to document once that issue lands.
- Status changed to Needs work
over 1 year ago 5:52am 6 September 2023 - ๐ณ๐ฟNew Zealand quietone
I think we missed a step, according the current process this needs to have a working patch to test the change.
All changes that can be tested with coder/phpcs. A working patch is to be provided with the issue. Committers will test the phpcs rule as part of the approval process.
And what happens to the existing sniff, currently excluded,
<exclude name="Drupal.Commenting.VariableComment.Missing"/>
- Status changed to RTBC
over 1 year ago 7:10am 6 September 2023 - ๐ฌ๐งUnited Kingdom catch
If we follow the new proposal at #3365085: Update the coding standards process on the project page โ then based on the current situation, we should have checked the existing sniff (which you just have), but we're not blocked on a new one.
Since
<exclude name="Drupal.Commenting.VariableComment.Missing"/>
is already disabled and therefore changing the rule won't break core's existing enabled ruleset.That would get us to:
If there is no existing phpcs rule for the change, then optionally a patch can be provided on the coding standards issue to implement the rule. Once the coding standards issue is fixed, an issue can be opened either against phpcs or coder to add support for the rule. This is to prevent issues being opened against coder/phpcs for rules which core might not adopt.
in the new process.
i.e. we should open an issue against coder asking for either a new sniff, or a modification to
<exclude name="Drupal.Commenting.VariableComment.Missing"/>
, as part of the process of marking this issue fixed.Based on that, moving back to RTBC.
- ๐ณ๐ฑNetherlands bbrala Netherlands
Confirmed the changes in the documentation. This can be marked as fixed.
- ๐ณ๐ฑNetherlands bbrala Netherlands
Fixing credits:
Kingdutch: made issue, discussed in slack.
quietone: managing this the last few months, and driving discussion.
larowlan: heping out with issue management and discussion
chx: discussion, and followup work.
bbrala: discussions and issue management
pfrenssen: work on docs
dww: work on docs - Status changed to Fixed
over 1 year ago 1:54pm 13 September 2023 - ๐ณ๐ฑNetherlands bbrala Netherlands
Marking as fixed. Lovely to see a codingstandards issue fixed. <3
Thanks everyone.
Automatically closed - issue fixed for 2 weeks with no activity.