Do not specify a category delimiter by default #13688
Conversation
tsteur
added
not-in-changelog
|
Guess it makes sense to set it empty by default. Regarding mentioning it in a changelog: Might it make sense to trigger a notification within Matomo after updating to explain the possible change to the super user once? |
|
I reckon it's not needed as long as it is mentioned clearly in the regular changelog. I suppose not too many people are too keen on how this feature worked previously and might maybe rather confuse people? |
|
Guess you're right. The page title report might not be one of the frequently used reports at all |
|
Looks like this change causes issues in displaying the "Page Name not defined" text, not sure why on first glance. |
|
pushed a fix that hopefully resolves the issue. I'll have a look at the failing tests and update them later. |
|
Things should work now. There were various bugs around the delimiter in the code and the code is quite hard to understand / messy. Also added to some tests the delimiter with |
| if (empty($name)) { | ||
| $name = self::getUnknownActionName($type); | ||
| } | ||
| return array(trim($name)); |
There was a problem hiding this comment.
Should we do the trim() before checking for an empty($name)?
There was a problem hiding this comment.
done, also realised empty but not be best if the name of a title is 0 (which might not happen but who knows)
| public static function setUpBeforeClass() | ||
| { | ||
| parent::setUpBeforeClass(); // TODO: Change the autogenerated stub | ||
| } |
There was a problem hiding this comment.
Not super important to me, but don't need this method.
| return $previous; | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Should we just change the tests w/ the new config or is there value in keeping the old behavior in all tests?
There was a problem hiding this comment.
Actually, it looks like only a couple of the tests have this, so I guess most use the new default?
There was a problem hiding this comment.
Most use default empty, but wanted to make sure some tests still use this behaviour to test it as well.
Doesn't require a note in the developer changelog but we should ideally mention this in the changelog as it is changing the behaviour for people that actually want to use it.
Tested it locally and worked.
fix #2956