A term like "0 years" is always tagged as soon as the word "years" is found, since array_filter() in the rules_autotag_split_text will remove the word "0", so the module will actually try to tagg "years".

CommentFileSizeAuthor
#7 rules_autotag-zero_term_issue-3080089-7-7.x-dev.patch2.06 KBsepgil
#5 rules_autotag-zero_term_issue-3080089-3-7.x-dev.patch2.26 KBsepgil
#2 rules_autotag-zero_term_issue-3080089-1-7.x-dev.patch2.25 KBsepgil
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sepgil created an issue. See original summary.

sepgil’s picture

sepgil CreditAttribution: sepgil at jobiqo - job board technology commented
FileSize
rules_autotag-zero_term_issue-3080089-1-7.x-dev.patch2.25 KB

Here is a patch that will fix the issue and add a unit test class for the rules_autotag_split_text() function.

sepgil’s picture

sepgil CreditAttribution: sepgil at jobiqo - job board technology commented
Status: Active » Needs review
mh86’s picture

mh86 CreditAttribution: mh86 as a volunteer and at jobiqo - job board technology commented
Status: Needs review » Needs work

Thx for the patch Sebastian!

I have two remarks:

* We should use drupal_strlen() instead of strlen()
* Why not re-use RulesAutotagTestCase for the test case? This way we would have all test cases in one place

sepgil’s picture

sepgil CreditAttribution: sepgil at jobiqo - job board technology commented
Status: Needs work » Needs review
FileSize
rules_autotag-zero_term_issue-3080089-3-7.x-dev.patch2.26 KB

Right, added a new patch for drupal_strlen().

Regarding the tests:
This are pure unit tests and using DrupalUnitTestCase allows them to run much faster than with DrupalWebTestCase, which takes a lot of time and resources.
Secondly the RulesAutotagTestCase class was failing for me, at line 15 when the rules_autotag module should be enabled. Not sure yet if that was a jobiqo specific problem or a general issue.

This way we would have all test cases in one place

The tests are grouped by the modules name, so I don't see the big advantage here.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi at jobiqo - job board technology commented
Status: Needs review » Needs work

Thanks, the tests make sense!

I think this can be simplified a lot:

  1. +++ b/rules_autotag.module
@@ -128,7 +128,11 @@ function rules_autotag_split_text($text) {
+  return array_filter(preg_split("/[" . RULES_AUTOTAG_SPLIT_REGEX . "]+/u", $text), function ($part) {
+    // We only want to keep words, i.e. values with at least one char. We can't
+    // use empty(), since this would remove "0"  which his valid.
+    return drupal_strlen($part) > 0;

    Instead of using array_filter we should remove it completely and pass the flag PREG_SPLIT_NO_EMPTY to preg_split(). That way we never get empty strings back and this is much simpler. 

  2. +++ b/rules_autotag.module
@@ -128,7 +128,11 @@ function rules_autotag_split_text($text) {
+    // use empty(), since this would remove "0"  which his valid.

    which is valid. 

  3. +++ b/rules_autotag_unit.test
@@ -0,0 +1,33 @@
+  }
+}

    There should be an empty line after the last method in a class.

sepgil’s picture

sepgil CreditAttribution: sepgil at jobiqo - job board technology commented
Status: Needs work » Needs review
FileSize
rules_autotag-zero_term_issue-3080089-7-7.x-dev.patch2.06 KB

Right, make sense. Here's a patch that replaces the array_filter with the flag.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi at jobiqo - job board technology commented
Status: Needs review » Reviewed & tested by the community

Super, thanks!

Added you as maintainer, please commit and create a new release.

  • sepgil committed 2008810 on 7.x-1.x 
    Issue #3080089 by sepgil: Wrong tagging for terms with 0
sepgil’s picture

sepgil CreditAttribution: sepgil at jobiqo - job board technology commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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