Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
The docblock for protected function createTerm() in file core/modules/taxonomy/src/Tests/Views/TaxonomyTestBase.php contains the odd reference to 'entity_create'. This needs to be investigated and fixed.
Is it a reference to the depreciated function entity_create()? If so, the @param comment needs to be updated to what is the preferred way of passing the settings to whatever has replaced entity_create().
Comment | File | Size | Author |
---|---|---|---|
#45 | 2627038-45.patch | 1.19 KB | ashhishhh |
Comments
Comment #2
Lars Toomre CreditAttribution: Lars Toomre as a volunteer commentedThis reference was encountered in #2608732: Some fixes for 'optional' parameter in core/modules comment #4.
Comment #3
Lars Toomre CreditAttribution: Lars Toomre as a volunteer commentedThere are a couple of other issues in this file that also may be addressed in this issue. Running Drupal standard coder_sniff against this file results in the following:
Comment #4
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedComment #5
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedComment #6
jordanpagewhite CreditAttribution: jordanpagewhite as a volunteer commentedComment #7
jordanpagewhite CreditAttribution: jordanpagewhite as a volunteer commentedThis passed the codesniff. I'm pretty sure we can use @inheritdoc for setUp. See other TaxonomyTestBase a directory down (with the same extends) for an example.
Comment #8
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedTry to solve all the problems in a single patch. Thanks for your time.
Comment #9
jordanpagewhite CreditAttribution: jordanpagewhite as a volunteer commented@snehi What should I interdiff against?
Comment #10
jhodgdonThanks! Looks like it fixes the code sniffer problems. This is not strictly a documentation issue since it also changes a line of code but I'm not going to worry about that (it's removing one space).
Comment #11
jordanpagewhite CreditAttribution: jordanpagewhite as a volunteer commentedI may have misinterpreted Lars' original task description. I see that the API docs suggest using the ::create() method (https://api.drupal.org/api/drupal/core!includes!entity.inc/function/enti...). However, it does seem like there are ::create() methods in the suggested location i.e. \Drupal\node\Entity\Node::create() . Can someone shed some light on this? Should we create a task to develop these ::create() methods?
Comment #12
jhodgdonHm. So your patch fixes the code sniffer issues in comment #3, which is great!
But I think you're right, it doesn't address the other stuff in the issue summary.
So here's the doc block the issue summary is referencing:
The code in this method does call entity_create(). Yes, it's a deprecated function, but this is a documentation issue... so let's just fix the documentation now. There may be an existing issue to remove calls to entity create somewhere but this isn't it.
So what we would need the patch to do, to fix this doc block, is:
a) Change (Optional) to (optional) -- lower-case -- to conform to our standards.
b) Change `entity_create` to say entity_create(), without the ` backticks and with () after the function name, which will turn it into a link.
Thanks!
Comment #13
jordanpagewhite CreditAttribution: jordanpagewhite as a volunteer commentedGreat, I have made those changes in this patch and interdiff. Let me know if I can make any other improvements.
Comment #14
jordanpagewhite CreditAttribution: jordanpagewhite as a volunteer commentedComment #15
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commented+1 for RTBC, All the mentioned tasks completed in this patch.
Comment #16
Lars Toomre CreditAttribution: Lars Toomre as a volunteer commentedThanks for the patch @jordanpagewhite. This is almost complete.
The one thing that I noticed is missing is what the default value is for the $settings variable. One has good docblock documentation when it is sufficient that one can use the function or method without referencing the code at all. In this case, a default value would help.
Comment #17
jordanpagewhite CreditAttribution: jordanpagewhite as a volunteer commented@Lars Toomre, got it. I've added that to this patch and interdiff.
Comment #18
jhodgdonThanks!
I have one concern:
This is *technically* true, but very misleading. If you look at the code, you will see that there are a number of defaults that are added to $settings, so really $settings is additions and overwrites of the default values.
Is there some way we could fix the documentation so it gets this across? I don't think we necessarily need to explain what the defaults are explicitly (although we could?), but at least saying that you are overriding some defaults would clue people in who care that they might need to look at the code to see what kind of taxonomy term is going to be created.
Comment #19
jordanpagewhite CreditAttribution: jordanpagewhite as a volunteer commentedIt seems to me that it would just be easier to read the code. Perhaps documenting the value of $settings is unnecessary?
Comment #20
jhodgdonYeah, I kind of agree on that... but the docs should still give you the information that there *are* default values that you are adding to and overriding, rather than implying $settings is all that are used, right?
Comment #21
jordanpagewhite CreditAttribution: jordanpagewhite as a volunteer commentedHmm. Yeah, I am not certain of the extent to which we should document each key-value pair in the array. Do you have an idea of how similar documentation issues have been resolved in the past?
Comment #22
jhodgdonAgain, I'm fine with *not* documenting the default values in $settings. But the documentation as it is now implies that only what you pass in for $settings is used to create the taxonomy term. That is not true. So let's reword the documentation so that it at least tells you that there are default values, and that you're adding to or overriding them. Does that make sense?
Comment #23
sudhanshug CreditAttribution: sudhanshug commentedComment #24
jhodgdon@sudhanshug: This issue is assigned to jordanpagewhite. Please do not upload patches on issues that are assigned to other people. Thanks!
Also that patch doesn't contain all the stuff that is in the previous patch, or resolve the remaining issues that we were working on. So... We welcome your willingness to contribute to the project, but please try to follow the guidelines that we have for contributing. See https://www.drupal.org/novice to get started. Please read that guide before jumping in. Thanks!
Comment #25
jordanpagewhite CreditAttribution: jordanpagewhite as a volunteer commentedComment #26
ashhishhh CreditAttribution: ashhishhh at Valuebound commentedComment #27
ashhishhh CreditAttribution: ashhishhh at Valuebound commentedVariable $vid doesn't exists here. I have changed this sentence too.
Comment #28
ashhishhh CreditAttribution: ashhishhh at Valuebound commentedComment #29
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedLooks ok to me +1 for RTBC
Comment #30
Manjit.Singh@ashhishhh Are you still working on it. In case if you done with the work please unassigned issue from yourself.
Comment #31
ashhishhh CreditAttribution: ashhishhh at Valuebound commentedComment #32
jhodgdonFor the future, it is actually beneficial if the person who works on the issue stays assigned until the patch is committed. So I would not ask someone to unassign themselves unless the patch has been marked "needs work" and there is no response for several days. It is much better to have just one person working on one issue at a time. Thanks!
Anyway... nice attempt but this latest patch needs some work:
Um. There is no documented parameter with name $vocabulary. So this line is not right, unless there is another parameter that isn't documented?
This isn't grammatical and doesn't make much sense. Please rewrite.
needs "an" in there.
Comment #33
ashhishhh CreditAttribution: ashhishhh at Valuebound commentedComment #34
ashhishhh CreditAttribution: ashhishhh at Valuebound commentedEarlier patches no more working for me.
Possible reason could be, one part has already been committed.
My patch has not included above.
Comment #35
jhodgdonThanks... still similar problems with this patch...
The vocabulary part is only accurate if the vid has not been overridden by $settings, and the random properties part is only accurate if those have not been overridden in $settings.
So this whole line is not really accurate. Please revise.
A @see here is a good idea. However, @see needs to come at the end of the doc block.
`entity_create` should be entity_create()
This sentence is garbled.
This is ... what is it supposed to mean? Doesn't make sense. You are documenting the default values of $settings that someone can overwrite. This documentation is not saying that at all. Please fix.
So... What I think we should say for $settings is something like:
(optional) An array of values to override the following default properties of the term:
And then list what the properties are and their default values.
Technically, there is no guarantee these are unique, they are just random.
id -> ID, and we should probably say what the default value is?
Comment #36
ashhishhh CreditAttribution: ashhishhh at Valuebound commentedComment #37
ashhishhh CreditAttribution: ashhishhh at Valuebound commentedThank you @jhodgdon, for this great review.
I worked on all except pt 1 & 7.
New value of vid is:
So I used self::$vocabulary. I couldn't improve this sentence further.
I renamed
random propertiesto random name as we are placing random values to name & description only.I don't see any default value.
Comment #38
jhodgdonThanks! Looking better.
I think we still need to fix this a bit though:
OK, obviously what I said in my previous review didn't make sense. Sorry about that!
What I was trying to say was that this sentence is only accurate if $settings does not override the vocabulary or name. So I think we shouldn't say this. We should instead just say:
Creates and returns a taxonomy term.
or something like that.
This is decent documentation now, thanks! The lines should be wrapped closer to 80 characters though.
The default value here, as you noted in this patch in the first line, is the ID from self::$vocabulary, right? Please document this.
I think that rather than having an @see here, with no real context, it would be better to put this class/member variable information into the 'vid' component of $settings.
Comment #39
Manjit.SinghComment #40
Manjit.Singh@jhodgdon I have addressed all points except the 4th one.
Comment #41
ashhishhh CreditAttribution: ashhishhh at Valuebound commentedComment #42
ashhishhh CreditAttribution: ashhishhh at Valuebound commented@Manjit, I did not find your patch correct.
Not only 4, you have not addressed pt 2 & 3 as well.
So I worked on #38 again. Here is the patch.
Comment #43
ashhishhh CreditAttribution: ashhishhh at Valuebound commentedComment #44
jhodgdonThanks! Almost there, but:
This is still not always true. Please read earlier reviews.
Comment #45
ashhishhh CreditAttribution: ashhishhh at Valuebound commentedComment #46
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedWhat about this ?
Comment #47
jhodgdonIn my opinion, #45 is much better than #46. #46 says "in vocabulary" but that is vague. The patch in #45 is actually quite good documentation, finally!
So to avoid any possibility of confusion, I am re-uploading the patch in #45 and hiding everything else. Thanks!
Comment #50
catchCommitted/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!