Skip to content

Allow annotations migration to work with both option and annotations tables#78

Merged
AltamashShaikh merged 4 commits into
5.x-devfrom
dev-13979
Sep 16, 2025
Merged

Allow annotations migration to work with both option and annotations tables#78
AltamashShaikh merged 4 commits into
5.x-devfrom
dev-13979

Conversation

@michalkleiner

@michalkleiner michalkleiner commented Sep 9, 2025

Copy link
Copy Markdown
Contributor

Description

As a security enhancement in core, annotations are being moved from the option table to their own annotations table. The class AnnotationList is also being removed as not needed, so that can be used as the deciding factor which way of migration to use.

Keeping the use statement for the removed AnnotationList class is ok in PHP.

Issue No

Relates to matomo-org/matomo#23564 and DEV-13979.

Checklist

  • [✔] Tested locally
  • [✔] New test case added/updated?
  • [NA] Are all newly added texts included via translation?
  • [NA] Are text sanitised properly? (Eg use of v-text v/s v-html for vue)
  • [NA] Version bumped?

@snake14 snake14 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @michalkleiner . I think the changes look good. Would you be able to adjust the tests to cover the new table when it's present?

@michalkleiner

Copy link
Copy Markdown
Contributor Author

@snake14 updated the migration to report the number of found annotations and the migration test to check for the number of migrated items when the AnnotationList class doesn't exist.

@michalkleiner

Copy link
Copy Markdown
Contributor Author

AI assistant referrer test failures addressed in a separate PR.

@michalkleiner michalkleiner requested review from a team and AltamashShaikh September 11, 2025 08:26

@snake14 snake14 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Michal. The changes look good to me 👍

Apologies for the delay in my review.

Edit: I left 1 comment with a question.

Comment thread tests/System/Commands/MigrateTest.php
@snake14

snake14 commented Sep 15, 2025

Copy link
Copy Markdown
Contributor

As this checks whether the class is present, I think that it's safe to merge before the core PR. Do you agree @AltamashShaikh ?

@AltamashShaikh AltamashShaikh merged commit afa9d33 into 5.x-dev Sep 16, 2025
6 checks passed
@AltamashShaikh AltamashShaikh deleted the dev-13979 branch September 16, 2025 02:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants