Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Some of functions in boostrap are missing the data type for the return. We should fix this. There also is some dead code which makes the core a little bit more complex to understand, so we should remove that as well.
Proposed resolution
Remaining tasks
User interface changes
None
API changes
None
Data model changes
None
Beta phase evaluation
Issue category | Bug since we don't maintain our own code standards (for documentation) |
---|---|
Issue priority | Normal |
Unfrozen changes | Unfrozen because it only changes documentation |
Comment | File | Size | Author |
---|---|---|---|
#30 | interdiff.txt | 642 bytes | Nitesh Pawar |
#30 | 2579099-30.patch | 1.1 KB | Nitesh Pawar |
#28 | interdiff-2579099-26-28.txt | 517 bytes | naveenvalecha |
#28 | 2579099-28.patch | 1.71 KB | naveenvalecha |
#26 | 2579099-26.patch | 1.71 KB | Nitesh Pawar |
Comments
Comment #2
googletorp CreditAttribution: googletorp as a volunteer and at Reveal IT commentedWhile cleaning up the documentation, my editor showed that we have a variable defined in drupal_get_filename() which isn't used, so I opted to delete this (for documentation purpose, to make the function easier to understand).
I don't know if we want to split that out, but seems like a lot to create an issue just to remove
Comment #3
googletorp CreditAttribution: googletorp as a volunteer and at Reveal IT commentedAlso I added an issue for the @todo, so we can keep track of it.
Comment #4
jhodgdonAgreed, $original_type is only declared in that one line and then never mentioned in any other line of bootstrap.inc. And most of your changes here look good, thanks!
A few minor things to fix:
boolean => bool
[when used as a type]
Normally instead of @return array, we'd prefer to have something like:
@return string[]
@return \Some\Class\Name[]
etc.
So if you know what type is in the array, that would be preferable.
Comment #5
cilefen CreditAttribution: cilefen commentedComment #6
jhodgdonLet's get the title actually spelled correctly. Third time's a charm. ;)
Comment #7
googletorp CreditAttribution: googletorp as a volunteer and at Reveal IT commented4.1 Changed boolean to bool (fun fact we have around 100 @return boolean and 1500 @return bool)
4.2 The function is returning an array of arrays, so that's why I opted for @return array
Comment #8
jhodgdonFair enough. Looks good!
Comment #9
xjmTechnically this shouldn't really be included in the scope of the issue. I confirmed that the
$original_type
parameter is not really used in any case.I think we also should not add the link to the @todo here because I think there must be an existing issue for this other than #2579097: Remove false-exposure of profiles as modules (about the way that profiles are treated as modules). Maybe we could repurpose that other issue to be about tracking down the correct issue and adding it to the @todo. :) Commented over there.
Comment #10
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedDone as suggested in #9.
Comment #11
googletorp CreditAttribution: googletorp as a volunteer and at Reveal IT commented@xjm makes sense, I'll do some investigation and post find findings unless some ones beats me to it.
Since the actually doc changes already has been approved and we are handling the comments on #9 I'm putting this back to RTBC.
Comment #12
tstoecklerWell, let's not actually remove the @todo please, it's still valid.
Comment #13
Nitesh Pawar CreditAttribution: Nitesh Pawar at Trigyn Technologies Ltd commentedInterdiff of #10
Comment #14
Nitesh Pawar CreditAttribution: Nitesh Pawar at Trigyn Technologies Ltd commentedComment #15
jhodgdonThis patch is still at Needs Work due to #12.
Comment #16
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedComment #17
jhodgdonThis is not a reroll. It needs a fix to the patch. See #12.
Comment #18
Nitesh Pawar CreditAttribution: Nitesh Pawar at Trigyn Technologies Ltd commentedNew patch on latest revision.
Comment #19
jhodgdonThanks, but this patch has problems that are not in the most recent patch. Please start with that patch and just leave the lines with the @todo in it.
Comment #20
ashhishhh CreditAttribution: ashhishhh at Valuebound commentedPatch given on #10 no more work.
In #18, I made only one change
Comment #21
jhodgdonThis is the return value of drupal_static_reset(). It is not an array. So I do not understand why you made this change? It is incorrect. Please put it back the way it was.
It would really help if people working on issues would follow the instructions given by reviewers, especially Drupal Core branch/documentation maintainers. Thanks!
Comment #22
ashhishhh CreditAttribution: ashhishhh at Valuebound commentedComment #23
jhodgdonONCE AGAIN, we need to return to the patch in #12, which was the closest patch that we had, and make sure EVERYTHING in there gets into the new patch, EXCEPT that we do not want to remove the @todo that was there. FOR SOME REASON no one has listened to this instruction. PLEASE do not ignore this again. The patches since then all have problems that I don't care to review. Thanks!
Comment #24
ashhishhh CreditAttribution: ashhishhh at Valuebound commented@jhodgdon, There is no patch available @ #12.
Can you check once more & specify, which one is close to final one.
Comment #25
jhodgdonSorry, see comment #12 for instructions. The most recent nearly OK patch is just before that in comment #10.
Comment #26
Nitesh Pawar CreditAttribution: Nitesh Pawar at Trigyn Technologies Ltd commentedhi jhodgdon,
I created patch on latest revision and removed @todo.
Comment #27
googletorp CreditAttribution: googletorp as a volunteer and at Reveal IT commentedGetting closer, not quite there yet.
It should be the data type, not the variable name after @return
Comment #28
naveenvalechaEdit : Addressed #27
Comment #29
jhodgdonAGAIN: we do NOT want to remove the @todo in this patch. Please leave the @todo as it is in the code. Sorry for shouting but can you PLEASE read the instructions!
I am pretty sure I have said this in a previous review too [yes, see #21], but I will try again.
This function's return value is NOT necessarily an array. If anything it is "mixed", but that is also misleading because it is returning a variable by reference, not some data. So I think we should not put a type here at all.
Comment #30
Nitesh Pawar CreditAttribution: Nitesh Pawar at Trigyn Technologies Ltd commentedComment #31
jhodgdonSorry. This issue is going nowhere, my instructions are always ignored, and each patch has more problems than the last.
And to top it off, we have new guidelines on issue scope which this issue doesn't comply with. So I'm just going to mark it as Won't Fix.
If you'd like to fix @return types, I think there may be a meta-issue about it, which you can search for. Meanwhile, see
https://www.drupal.org/core/scope
for the scope guidelines. Sorry, but this is a Won't Fix as it is.