Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Segment with 'Does not contain' operand is invalid if comma is in value #19617

Closed
samjf opened this issue Aug 11, 2022 · 1 comment · Fixed by #21529
Closed

Segment with 'Does not contain' operand is invalid if comma is in value #19617

samjf opened this issue Aug 11, 2022 · 1 comment · Fixed by #21529
Labels
Bug For errors / faults / flaws / inconsistencies etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Milestone

Comments

@samjf
Copy link
Contributor

samjf commented Aug 11, 2022

When creating a new segment with a 'Does not contain' operator, if your value contains a comma then when an archive is run it will return "The segment condition ' After comma' is not valid."

Expected Behavior

Commas should be allowed as a valid character in the filter value

Current Behavior

A segment condition is not valid error message is display.

Possible Solution

I believe because it can be saved as a segment, but only fails on testing that the issue is not in the initial validation.
A big of debugging showed that getCleanedExpression function might be near the fault. This is likely caused by URL decoding done before parsing the definition or something.

Steps to Reproduce (for Bugs)

image
image

  1. Create a new segment in the drop down
  2. Choose the following options LHS: Event Name, middle: 'Does not contain', RHS: 'Before comma, after comma'
  3. Press the 'Test' button

The following is my segment definition from my DB: eventName!@Before%253A%2520comma%252C%2520After%2520comma

Context

This has blocked an instance from finishing archiving.

The error itself will not occur when you save the Segment, but only when it is tested in the segment form or run during the archive.

Your Environment

  • Matomo Version: 4.10.1
  • PHP Version: 8.0
  • Server Operating System: Linux / Mac
  • Additionally installed plugins:
  • Browser: Firefox
  • Operating System:
@samjf samjf added the Potential Bug Something that might be a bug, but needs validation and confirmation it can be reproduced. label Aug 11, 2022
@sgiehl
Copy link
Member

sgiehl commented Aug 12, 2022

This should indeed be caused by the url decoding. We had some issues in the past with segments being encoded too often or to less, so they didn't match the defined segments in the database when comparing. We might still perform url decode twice, to ensure it's not encoded anymore. Might not be that easy to get rid of it.
The "simplest" solution I currently can think of, would be to run some custom encoding on the segment value and replace characters that might cause problems with some placeholders we can later replace again safely. But that might take some time to implement as well.

@sgiehl sgiehl added Bug For errors / faults / flaws / inconsistencies etc. and removed Potential Bug Something that might be a bug, but needs validation and confirmation it can be reproduced. labels Aug 12, 2022
@sgiehl sgiehl added this to the For Prioritization milestone Aug 12, 2022
@jane-twizel jane-twizel added the Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. label Aug 17, 2022
@mneudert mneudert linked a pull request Nov 13, 2023 that will close this issue
11 tasks
@mneudert mneudert modified the milestones: For Prioritization, 4.16.0 Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants