@samjf opened this Issue on August 11th 2022 Contributor

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!<a class='mention' href='https://github.com/Before'>@Before</a>%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:
@sgiehl commented on August 12th 2022 Member

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.

Powered by GitHub Issue Mirror