Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider sendNotificationMessage Hook #2231

Merged
merged 1 commit into from
Jul 6, 2021
Merged

Consider sendNotificationMessage Hook #2231

merged 1 commit into from
Jul 6, 2021

Conversation

c4y
Copy link
Contributor

@c4y c4y commented Jun 16, 2021

Since Isotope uses Notification-Center it should support sendNotificationMessage Hook from Notification-Center. With this Bundle you can send different notifications dependant on product: https://github.com/postyou/isotope_notification_product_extension
This does actually not work because postsale.php removes the Hook. See #2174

Since Isotope uses Notification-Center it should support sendNotificationMessage Hook from Notification-Center. With this Bundle you can send different notifications dependant on product: https://github.com/postyou/isotope_notification_product_extension
This does actually not work because postsale.php removes the Hook. See isotope#2174
@fritzmg
Copy link
Contributor

fritzmg commented Jun 16, 2021

@aschempp why does the postsale entry point remove hooks at all? Shouldn't it only remove hooks that are known to cause issues for whatever reason?

@aschempp aschempp added the bug label Jun 17, 2021
@aschempp aschempp added this to the 2.7 milestone Jun 17, 2021
@aschempp
Copy link
Member

@c4y thanks for the PR, I'll merge this in the next release.

@fritzmg because there were various issue with hooks due to the fact that not the full system is initialized/no actual page is there.

@bennyborn
Copy link

@aschempp While I welcome the addition of sendNotificationMessage to the whitelist I still think this might be the wrong way. The core itself comes with a manageable amount of hooks so why not just create a blacklist with the hooks known to cause trouble? I'm pretty certain there will be more problems using a whitelist approach in the future.

@aschempp
Copy link
Member

aschempp commented Jul 6, 2021

The postsale process should not be considered a safe environment for any hook we don't know we support it. There have been plenty of random issues with it and third-party extensions, and the current solution seems to work fine.

@aschempp aschempp merged commit a4899c4 into isotope:2.7 Jul 6, 2021
@aschempp aschempp modified the milestones: 2.7, 2.7.3 Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants