When saving a node bundle type that does NOT have SearchInText settings saved, an error is thrown in entityPresave(). Because $settings is derived from ThirdPartySettings that are not set, $settings['autotagger_tag_on_create_only'] returns a null value on the array key. We need to check to see if $settings is empty or not before proceeding.

Issue fork autotagger-3425276

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

brooke_heaton created an issue. See original summary.

brooke_heaton opened merge request !17

brooke_heaton’s picture

brooke_heaton commented
Assigned: Unassigned » brooke_heaton
Status: Active » Needs review

Merge request submitted to 1.0.x branch.

brooke_heaton’s picture

brooke_heaton commented
Issue summary: View changes
alvar0hurtad0’s picture

alvar0hurtad0
he/him
Primary language Spanish
Location Cáceres
commented
Status: Needs review » Needs work

Hello Brooke,

Thank you very much for the MR and raising the issue. Looks good to me but it'll be great if you could use a ternary operation in order to avoid an extra indentation level.

it's something we're trying to do in the whole codebase and it'll make it more homogeneous.

Thank you very much.

brooke_heaton’s picture

brooke_heaton commented

Will do!

brooke_heaton’s picture

brooke_heaton commented

So I don't think a ternary is possible here. We actually want the entire code below to be conditional and we don't want to run ->tagNode whatsoever if there are no settings. I don't see a way to apply a ternary here, sorry. 

 if (isset($settings)) {

      $create_only = $settings['autotagger_tag_on_create_only'];

      if ($node->isNew() || !$create_only) {
        // If the node is first being created, or the node is set to not only tag
        // on new, then go ahead and tag it.
        $this->tagNode($node);
      }
brooke_heaton’s picture

brooke_heaton commented
Status: Needs work » Needs review
alvar0hurtad0’s picture

alvar0hurtad0
he/him
Primary language Spanish
Location Cáceres
commented
Assigned: brooke_heaton » alvar0hurtad0
Status: Needs review » Active

let me try :)

alvar0hurtad0’s picture

alvar0hurtad0
he/him
Primary language Spanish
Location Cáceres
commented

sorry, wrong comment.

  • alvar0hurtad0 committed 303050ed on 1.0.x 
    Issue #3425276 by brooke_heaton: Warning: Trying to access array offset...
alvar0hurtad0’s picture

alvar0hurtad0
he/him
Primary language Spanish
Location Cáceres
commented
Status: Active » Fixed

Thanks @brooke_heaton, I've submited a commit that should fix this issue.

I'll create a new release including also #3424520

alvar0hurtad0’s picture

alvar0hurtad0
he/him
Primary language Spanish
Location Cáceres
commented
Assigned: alvar0hurtad0 » Unassigned

removing the assigned field.

brooke_heaton’s picture

brooke_heaton commented
Status: Fixed » Needs work

Nope. That's not the problem.

See, by setting $create_only to FALSE, you are STILL running a function that should not be ran and results in additional errors. 

Warning: Trying to access array offset on value of type null in Drupal\autotagger_search_in_text\Plugin\Autotagger\SearchInTextFields->tagNode() (line 209 of modules/contrib/autotagger/modules/autotagger_search_in_text/src/Plugin/Autotagger/SearchInTextFields.php).
Warning: Trying to access array offset on value of type null in Drupal\autotagger_search_in_text\Plugin\Autotagger\SearchInTextFields->tagNode() (line 210 of modules/contrib/autotagger/modules/autotagger_search_in_text/src/Plugin/Autotagger/SearchInTextFields.php).

We want to wrap this in a conditional and NOT run this if there are no settings on a bundle. 

 if ($node->isNew() || !$create_only) {
          // If the node is first being created, or the node is set to not only tag
          // on new, then go ahead and tag it.
            $this->tagNode($node);
        }

If there are no settings on a bundle, why are we tagging the node? The node has no settings and the node is new.

Your ternary is nice and all, but the goal here should be to prevent tagging of nodes of a type that do not have any settings. For that you need a conditional statement to prevent running $this->tagNode if $settings['autotagger_tag_on_create_only'] returns FALSE. BTW Drupal coding standards should have PHP Constants like FALSE in ALL CAPS.

  • alvar0hurtad0 committed bbb0d3a4 on 1.0.x 
    Issue #3425276 by brooke_heaton: Warning: Trying to access array offset...
alvar0hurtad0’s picture

alvar0hurtad0
he/him
Primary language Spanish
Location Cáceres
commented
Status: Needs work » Needs review

True, thanks for the heads-up.

Now I understand the issue, and I think is fixed.

https://git.drupalcode.org/project/autotagger/-/commit/bbb0d3a47222b42f5...

alvar0hurtad0’s picture

alvar0hurtad0
he/him
Primary language Spanish
Location Cáceres
commented
Status: Needs review » Fixed
brooke_heaton’s picture

brooke_heaton commented

Sure. Works for me.

alvar0hurtad0’s picture

alvar0hurtad0
he/him
Primary language Spanish
Location Cáceres
commented

Thanks a lot Brooke, for me is a great feeling to know the module is useful for someone else.

Status: Fixed » Closed (fixed)

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