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

Remove SonataNotificationBundle from dependencies and code #1415

Closed
7 tasks done
Tracked by #1416
eerison opened this issue Apr 22, 2022 · 10 comments
Closed
7 tasks done
Tracked by #1416

Remove SonataNotificationBundle from dependencies and code #1415

eerison opened this issue Apr 22, 2022 · 10 comments
Labels
Milestone

Comments

@eerison
Copy link
Contributor

eerison commented Apr 22, 2022

Remove SonataNotificationBundle

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

Create tickets

@eerison
Copy link
Contributor Author

eerison commented Apr 22, 2022

I'm on it!

@eerison
Copy link
Contributor Author

eerison commented Apr 27, 2022

Hey @core23

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

#1238 (comment)

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
Copy link
Contributor Author

eerison commented Apr 27, 2022

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 added a commit to eerison/SonataPageBundle that referenced this issue Apr 28, 2022
@eerison
Copy link
Contributor Author

eerison commented Apr 28, 2022

Started to deprecate NotificationBundle in #1418

@core23
Copy link
Member

core23 commented Apr 30, 2022

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
Copy link
Contributor Author

eerison commented Apr 30, 2022

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.

eerison added a commit to eerison/SonataPageBundle that referenced this issue May 18, 2022
eerison added a commit to eerison/SonataPageBundle that referenced this issue May 18, 2022
eerison added a commit to eerison/SonataPageBundle that referenced this issue Jun 8, 2022
eerison added a commit to eerison/SonataPageBundle that referenced this issue Jun 8, 2022
treehousedevelopers pushed a commit to treehousedevelopers/SonataPageBundle that referenced this issue Jun 10, 2022
@mareckigit
Copy link

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
Copy link
Contributor Author

eerison commented Jun 15, 2022

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: #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: #1418

@mareckigit
Copy link

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants