When I tried to apply this change locally, I got this error
An error occurred when trying to change the field 'name' via ALTER TABLE `log_action` CHANGE `name` `name` VARCHAR(65000) CHARACTER SET utf8 COLLATE utf8_general_ci NULL DEFAULT '' MySQL said: Column length too big for column 'name' (max = 21845); use BLOB or TEXT instead
It seems the column length is too big.
log_conversion.url even only
varchar(16000) works since there are so many more columns. I reckon it might be best to leave it as
text. It be not good to have different lengths of url fields and not sure if by using
log_conversion we might prevent in the future our chances of adding more columns...
Not sure how it's related to this PR, but the tests appear to be failing
Not understanding it... thought maybe we use really long action names but that doesn't seem to be the case in https://travis-ci.org/matomo-org/matomo/jobs/580980359#L848
Since we truncate URLs after 1024 characters by default, which can be customised in config.ini.php see: https://matomo.org/faq/troubleshooting/faq_21239/
it should be enough to create the VARCHAR with 4096 characters or so? 16k characters seems overkill and also afraid it could prevent us adding new columns in the future.
Changed it to
4096 and fixed the tests.
The results in
tests/PHPUnit/System/expected/ are correct. It is only the sorting that slightly changed because of the
varchar when there is eg the same amount of visits.
The results in
plugins/Contents/tests/System/expected/ are correct too. Took me quite some time to look into it and 100% understand things. The
contentTarget for example changed because there are multiple impressions with the same content name but different targets see https://github.com/matomo-org/matomo/blob/3.12.0-b3/plugins/Contents/tests/Fixtures/TwoVisitsWithContents.php#L79-L84 where we use
'Click to download Piwik now', '' but also
'Click to download Piwik now', 'http://piwik.org/download'.
We actually do not show the content target in the UI (which would make it 3 level report). We're currently for some reason adding
contentTarget to the report but the contentTarget value we use in the report is
random if there are multiple content names/pieces with different targets. So no logic changed there and things are the same. Ideally at some point we may want to look at removing content target from the report and / or implement targets correctly see https://github.com/matomo-org/matomo/issues/6267 Not changing any logic there for now for this.
Ready for another review. Hoping the tests pass now.
Still one test failure, but I think it's expected?
@diosmosis it seems the tests are failing now randomly and have different IDs on every request or different results in general :( the same tests are always failing in different ways. We'll need to see how this can be fixed... will likely take quite a while.
@katebutler can you also update the expected test results so the tests pass?
@katebutler is maybe this test failing still randomly? https://travis-ci.org/matomo-org/matomo/jobs/592761199#L820-L850
If that's the case we may need to have a secondary sort order column in the Sort datatable filter. This could fix this issue also for other reports in the future in https://github.com/matomo-org/matomo/blob/3.12.0-b4/core/DataTable/Filter/Truncate.php#L97 I reckon it could be good to enable
$doSortBySecondaryColumn = true parameter