Hi, I noticed that the response code condition isn't being respected correctly on the page.

It appears that this may be due to the strict type check in the condition class. Specifically, the getStatusCode() method is returning the status code as an integer, whereas the response codes from the settings are in string format.

See image

img

CommentFileSizeAuthor
#2 3353846-2-response-code-condition-not-respected.patch694 bytessker101
iShot_2023-04-12_16.50.52.png199.02 KBsker101

Issue fork google_tag-3353846

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

sker101 created an issue. See original summary.

sker101’s picture

sker101 commented
StatusFileSize
new 3353846-2-response-code-condition-not-respected.patch694 bytes

Here's a patch that force casts the status code into string

sker101’s picture

sker101 commented
Status: Active » Needs review

changing status to "Needs Review"

hanoii’s picture

hanoii
Primary language Spanish
Location 🇦🇷UTC-3
commented

If we are going to cast, isn't it better to remove the strict comparison instead?

sker101’s picture

sker101 commented

Although I'm unsure of the reason behind adding the strict comparison initially, personally, I believe that doing strict comparison is a beneficial practice in general.

hanoii’s picture

hanoii
Primary language Spanish
Location 🇦🇷UTC-3
commented

But if you are casting you are removing any potential benefit of the strict comparison

amangrover90 made their first commit to this issue’s fork.

amangrover90 opened merge request !35

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Needs review » Reviewed & tested by the community

I believe that doing strict comparison is a beneficial practice in general.

I agree, unless it's Drupal and dealing with integers.

MR fixes 200 response code checks as well.

  • mglaman committed d63fb1a1 on 2.0.x authored by amangrover90 
    Issue #3353846 by amangrover90, sker101: Response code condition setting...
mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Reviewed & tested by the community » Fixed

Merged! We're working on a few other follow ups before making a new release.

Status: Fixed » Closed (fixed)

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