Add event to alter delete website confirmation

[AltamashShaikh]

Description:

When a website is about to be deleted Matomo currently shows a confirmation.
As users might not be aware which additional plugin data might be removed with that, plugins should have the possibility to add some additional information/warning to that confirmation.

This PR introduces a new event SitesManager.getMessagesToWarnOnSiteRemoval, which allows plugins to provide additional text blocks that should be displayed within the modal.

Fixes: #PG-3574

Review

• Functional review done
• Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
• Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
• Security review done
• Wording review done
• Code review done
• Tests were added if useful/possible
• Reviewed for breaking changes
• Developer changelog updated if needed
• Documentation added if needed
• Existing documentation updated if needed

[sgiehl]

Hey @AltamashShaikh,
I've discussed the solution again in core team. As mentioned before the "perfect" solution would be using a javascript event, that plugins like the TagManager could easily use to add content to the confirmation. But as we currently do not have something like a global javascript event registry it is not really possible to solve it that way.
Such a javascript event registry would be beneficial for a lot other use cases as well, but core team would need to plan and work on that separately.

To be able to allow altering the removal confirmation already we might indeed need to go with a similar workaround like you implemented. But from our point of view the implementation - especially in TagManager - is too unflexible.
You are currently fully replacing the text of the confirmation. That for sure works, but if another plugin would aim to do the same the result is completely unpredictable. The plugin, that would be loaded last, would most likely "win the race" and place it's content, while the content of other plugins is simply overwritten.
When introducing new events you should always consider that other plugins might want to use those events as well.
My approach here would have been to have an event that only provides sort of a list of things to warn that the will be removed as well and keep a more general basic message as introduction.
So there would be an event like getThingsToWarnOnSiteRemoval, which receives the idSite and a variable, prefilled with an empty array. TagManager can then add the items that will be removed to that array. If the list contains items after calling the event, core would adjust the message to add them accordingly.

[AltamashShaikh]

roducing new events you should always consider that other plugins might want to use those events as well.
My approach here would have been to have an event that only provides sort of a list of things to warn that the will be removed as well and keep a more general basic message as introductio

@sgiehl I changed the event to pass an array with a default message that as the first key and plugins can add into the array of update the first key as required, will that work for you guys ??

[sgiehl]

I've left a couple of comments for further improvements. Let me know if you have questions on anything...

[AltamashShaikh]

@sgiehl I have updated the code as per your suggestion if all okay please don't merge as I need to get the text checked from product once to ensure if we need to change anything in core.

[AltamashShaikh]

@sgiehl All good from product team to merge this 👍
Please review and merge this

[sgiehl]

We should add a comment above the event to document what it is good for. Like it'S e.g. the case here:

matomo/core/Scheduler/Scheduler.php

Lines 157 to 166 in 9a3ef94

	/**
	* Triggered before a task is executed.
	*
	* A plugin can listen to it and modify whether a specific task should be executed or not. This way
	* you can force certain tasks to be executed more often or for example to be never executed.
	*
	* @param bool &$shouldExecuteTask Decides whether the task will be executed.
	* @param Task $task The task that is about to be executed.
	*/
	Piwik::postEvent('ScheduledTasks.shouldExecuteTask', array(&$shouldExecuteTask, $task));

And when looking at other events it seems we use to pass the referenced parameter first, and then the other. So you may switch idSite and messages

[AltamashShaikh]

@sgiehl Added as suggested 👍