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().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lars Toomre created an issue. See original summary.

Lars Toomre’s picture

Lars Toomre’s picture

There 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:

------------------------------------------------------------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 2 LINES
------------------------------------------------------------------------------------------------------------------------------------
 58 | ERROR | [x] Missing function doc comment (Drupal.Commenting.FunctionComment.Missing)
 85 | ERROR | [x] Expected 1 space after comma in function call; 2 found
    |       |     (Generic.Functions.FunctionCallArgumentSpacing.TooMuchSpaceAfterComma)
 85 | ERROR | [x] Expected one space after the comma, 2 found (Drupal.WhiteSpace.Comma.TooManySpaces)
------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------------------------------
snehi’s picture

Assigned: Unassigned » snehi
snehi’s picture

Assigned: snehi » Unassigned
jordanpagewhite’s picture

Assigned: Unassigned » jordanpagewhite
jordanpagewhite’s picture

Status: Active » Needs review
FileSize
836 bytes

This 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.

snehi’s picture

Status: Needs review » Needs work

Try to solve all the problems in a single patch. Thanks for your time.

jordanpagewhite’s picture

@snehi What should I interdiff against?

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community

Thanks! 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).

jordanpagewhite’s picture

I 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?

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

Hm. 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:

  /**
   * Returns a new term with random properties in vocabulary $vid.
   *
   * @param array $settings
   *   (Optional) An associative array of settings to pass to `entity_create`.
   *
   * @return \Drupal\taxonomy\Entity\Term
   *   The created taxonomy term.
   */

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!

jordanpagewhite’s picture

Great, I have made those changes in this patch and interdiff. Let me know if I can make any other improvements.

jordanpagewhite’s picture

Status: Needs work » Needs review
snehi’s picture

+1 for RTBC, All the mentioned tasks completed in this patch.

Lars Toomre’s picture

Status: Needs review » Needs work

Thanks 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.

jordanpagewhite’s picture

Status: Needs work » Needs review
FileSize
1.25 KB
588 bytes

@Lars Toomre, got it. I've added that to this patch and interdiff.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks!

I have one concern:

+++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyTestBase.php
@@ -122,7 +125,8 @@ protected function mockStandardInstall() {
    * @param array $settings
-   *   (Optional) An associative array of settings to pass to `entity_create`.
+   *   (optional) An associative array of settings to pass to entity_create().
+   *   Defaults to empty array.

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.

jordanpagewhite’s picture

It seems to me that it would just be easier to read the code. Perhaps documenting the value of $settings is unnecessary?

jhodgdon’s picture

Yeah, 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?

jordanpagewhite’s picture

Hmm. 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?

jhodgdon’s picture

Again, 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?

sudhanshug’s picture

jhodgdon’s picture

@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!

jordanpagewhite’s picture

Assigned: jordanpagewhite » Unassigned
ashhishhh’s picture

Assigned: Unassigned » ashhishhh
ashhishhh’s picture

* Returns a new term with random properties in vocabulary $vid.

Variable $vid doesn't exists here. I have changed this sentence too.

ashhishhh’s picture

Status: Needs work » Needs review
snehi’s picture

Looks ok to me +1 for RTBC

Manjit.Singh’s picture

@ashhishhh Are you still working on it. In case if you done with the work please unassigned issue from yourself.

ashhishhh’s picture

Assigned: ashhishhh » Unassigned
jhodgdon’s picture

Status: Needs review » Needs work

For 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:

  1. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyTestBase.php
    @@ -119,10 +122,18 @@ protected function mockStandardInstall() {
    +   * Returns a new term in vocabulary $vocabulary with random properties.
    

    Um. There is no documented parameter with name $vocabulary. So this line is not right, unless there is another parameter that isn't documented?

  2. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyTestBase.php
    @@ -119,10 +122,18 @@ protected function mockStandardInstall() {
    +   *   (optional) An associative array.
    +   *   To make new term unique, few properties requires unique value.
    +   *   Overwritten keys of $settings before passing to entity_create():
    

    This isn't grammatical and doesn't make much sense. Please rewrite.

  3. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyTestBase.php
    @@ -119,10 +122,18 @@ protected function mockStandardInstall() {
    +   *   Defaults to empty array.
    

    needs "an" in there.

ashhishhh’s picture

Assigned: Unassigned » ashhishhh
ashhishhh’s picture

Status: Needs work » Needs review
FileSize
1.4 KB
1.62 KB

Earlier patches no more working for me.
Possible reason could be, one part has already been committed.

-    $this->vocabulary = entity_create('taxonomy_vocabulary',  array(
+    $this->vocabulary = entity_create('taxonomy_vocabulary', array(

My patch has not included above.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks... still similar problems with this patch...

  1. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyTestBase.php
    @@ -119,10 +122,20 @@ protected function mockStandardInstall() {
    +   * Returns a new term in vocabulary self::$vocabulary with random properties.
    

    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.

  2. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyTestBase.php
    @@ -119,10 +122,20 @@ protected function mockStandardInstall() {
    +   * @see \Drupal\taxonomy\Tests\Views\TaxonomyTestBase::$vocabulary
    

    A @see here is a good idea. However, @see needs to come at the end of the doc block.

  3. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyTestBase.php
    @@ -119,10 +122,20 @@ protected function mockStandardInstall() {
    +   *   (optional) An associative array of settings to pass to `entity_create`.
    

    `entity_create` should be entity_create()

  4. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyTestBase.php
    @@ -119,10 +122,20 @@ protected function mockStandardInstall() {
    +   *   To create taxonomy term, few properties require unique value.
    

    This sentence is garbled.

  5. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyTestBase.php
    @@ -119,10 +122,20 @@ protected function mockStandardInstall() {
    +   *   Overwritten keys of $settings, before passing to entity_create():
    

    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.

  6. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyTestBase.php
    @@ -119,10 +122,20 @@ protected function mockStandardInstall() {
    +   *   - name: A unique random string.
    +   *   - description: A unique random string.
    

    Technically, there is no guarantee these are unique, they are just random.

  7. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyTestBase.php
    @@ -119,10 +122,20 @@ protected function mockStandardInstall() {
    +   *   - vid: Vocabulary id.
    

    id -> ID, and we should probably say what the default value is?

ashhishhh’s picture

ashhishhh’s picture

Status: Needs work » Needs review
FileSize
1.37 KB
1.47 KB

Thank you @jhodgdon, for this great review.
I worked on all except pt 1 & 7.

  1. 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.

    New value of vid is:

    'vid' => $this->vocabulary->id(),
    

    So I used self::$vocabulary. I couldn't improve this sentence further.
    I renamed random properties to random name as we are placing random values to name & description only.

  2. For pt 7
    id -> ID, and we should probably say what the default value is?

    I don't see any default value.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! Looking better.

I think we still need to fix this a bit though:

  1. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyTestBase.php
    @@ -119,13 +122,22 @@ protected function mockStandardInstall() {
    +   * Returns a new term in vocabulary self::$vocabulary with random name.
    

    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.

  2. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyTestBase.php
    @@ -119,13 +122,22 @@ protected function mockStandardInstall() {
    +   *   (optional) An array of values to override
    +   *   the following default properties of the term:
    

    This is decent documentation now, thanks! The lines should be wrapped closer to 80 characters though.

  3. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyTestBase.php
    @@ -119,13 +122,22 @@ protected function mockStandardInstall() {
    +   *   - vid: Vocabulary ID.
    

    The default value here, as you noted in this patch in the first line, is the ID from self::$vocabulary, right? Please document this.

  4. +++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyTestBase.php
    @@ -119,13 +122,22 @@ protected function mockStandardInstall() {
    +   *
    +   * @see \Drupal\taxonomy\Tests\Views\TaxonomyTestBase::$vocabulary
    

    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.

Manjit.Singh’s picture

Assigned: ashhishhh » Manjit.Singh
Manjit.Singh’s picture

Assigned: Manjit.Singh » Unassigned
FileSize
1.62 KB
863 bytes

@jhodgdon I have addressed all points except the 4th one.

ashhishhh’s picture

Assigned: Unassigned » ashhishhh
ashhishhh’s picture

@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.

ashhishhh’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Thanks! Almost there, but:

+++ b/core/modules/taxonomy/src/Tests/Views/TaxonomyTestBase.php
@@ -119,10 +122,17 @@ protected function mockStandardInstall() {
+   * Creates and returns a taxonomy term with random name.

This is still not always true. Please read earlier reviews.

ashhishhh’s picture

Status: Needs work » Needs review
FileSize
1.19 KB
504 bytes
snehi’s picture

What about this ?

jhodgdon’s picture

In 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!

The last submitted patch, 46: 2627038-46.patch, failed testing.

  • catch committed 30c5c2d on 8.1.x
    Issue #2627038 by ashhishhh, jordanpagewhite, snehi, Manjit.Singh,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

  • catch committed 3d64656 on 8.0.x
    Issue #2627038 by ashhishhh, jordanpagewhite, snehi, Manjit.Singh,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.