sonata-project / SonataPageBundle

This bundle provides a Site and Page management through container and block services
https://docs.sonata-project.org/projects/SonataPageBundle
MIT License
216 stars 209 forks source link

Remove SonataNotificationBundle from dependencies and code #1415

Closed eerison closed 2 years ago

eerison commented 2 years ago

Remove SonataNotificationBundle

As discussed in #1410 , we need to remove SonataNotificationBundle from the next major release

Create tickets

eerison commented 2 years ago

I'm on it!

eerison commented 2 years ago

Hey @core23

I didn't see your Issue regarding NotificationBundle #1238 , my bad

https://github.com/sonata-project/SonataPageBundle/issues/1238#issue-749833646

But if I understand correct we do not need to use symfony/messager component, or is it necessary?

could you give some example(s) how was you thinking to remove this decency?, I guess you understand this functionality better then me 😄

eerison commented 2 years ago

The original idea was to allow async snapshot creation / cleanup.

do you mean create and remove snapshots from the database?

To allow deprecating the usage, we could introduce a new SnapshotManagerInterface with two methods createSnapshotsForSite and cleanupSnashotsForSite.

do you know where we are managing this logical (to create and remove snapshots) using NotificationBundle? just to have a better understand :)

Edit 1: Hmmm I saw that it was already created SnapshotManagerInterface and SnapshotManager, I'll take a look why we are still using NotificationBundle in the code ;)

eerison commented 2 years ago

Started to deprecate NotificationBundle in #1418

core23 commented 2 years ago

But if I understand correct we do not need to use symfony/messager component, or is it necessary?

could you give some example(s) how was you thinking to remove this decency?, I guess you understand this functionality better then me 😄

The idea of my issue was to remove the whole async processing, because a "normal" website should be able to create snapshots on the fly.

The notification bundle or messager component adds a complexity to this bundle that is not necessary.

eerison commented 2 years ago

But if I understand correct we do not need to use symfony/messager component, or is it necessary? could you give some example(s) how was you thinking to remove this decency?, I guess you understand this functionality better then me 😄

The idea of my issue was to remove the whole async processing, because a "normal" website should be able to create snapshots on the fly.

The notification bundle or messager component adds a complexity to this bundle that is not necessary.

Hey @core23 thank you for reply, make sense to remove this async, because after the project is on production, we usually publish one page when it's needed.

mareckigit commented 2 years ago

I have removed Sonata Notification in my fork: https://github.com/treehousedevelopers/SonataPageBundle/commits/4.x. You can use it with Symfony 5.4 but I'm still working on it.

eerison commented 2 years ago

I have removed Sonata Notification in my fork: https://github.com/treehousedevelopers/SonataPageBundle/commits/4.x. You can use it with Symfony 5.4 but I'm still working on it.

Hi @mareckigit

I didn't find what commit, you are removing the code, But as I can see you are removing this directly from 4.x and we need to create some BC compatibility to the code works in 3.x release!

But we should save our energy to work in different tasks, as I'm working in remove this notification bundle you could work in other task that, as far as I know anyone is working on that, for example: https://github.com/sonata-project/SonataPageBundle/issues/1096

were you working only in notification bundle or something else? if you worked in other thing could you provide a Pull request with the changes?

Note 1: try to provide small PRs, big PR with a lot of things probably it won't be approved 👀 Note 2: can you comment in the issue before start to code, it'll avoid 2 people work in the same task. Note 3: it's my PR for removing notification bundle: https://github.com/sonata-project/SonataPageBundle/pull/1418

mareckigit commented 2 years ago

My goal is to prepare Sonata Page version that will be work with Symfony 5.4. I do my changes in branch 4.x because I expect that is dedicated for Symfony > 4. My fork includes:

jordisala1991 commented 2 years ago

This can be closed, the remaining cleanup does not belong to the usage of SonataNotificationBundle.