Closed (won't fix)
Project:
Drupal core
Version:
8.0.x-dev
Component:
documentation
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
2 Oct 2015 at 14:07 UTC
Updated:
19 Jan 2016 at 20:03 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
googletorp 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 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 commentedComment #6
jhodgdonLet's get the title actually spelled correctly. Third time's a charm. ;)
Comment #7
googletorp 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_typeparameter 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 commentedDone as suggested in #9.
Comment #11
googletorp 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 commentedInterdiff of #10
Comment #14
nitesh pawar commentedComment #15
jhodgdonThis patch is still at Needs Work due to #12.
Comment #16
snehi commentedComment #17
jhodgdonThis is not a reroll. It needs a fix to the patch. See #12.
Comment #18
nitesh pawar 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 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 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 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 commentedhi jhodgdon,
I created patch on latest revision and removed @todo.
Comment #27
googletorp 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 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.