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

Decouple and Deprecate notification bundle from create snapshot code #1418

Merged
merged 18 commits into from
Jul 7, 2022

Conversation

eerison
Copy link
Contributor

@eerison eerison commented Apr 28, 2022

Decouple and Deprecate notification bundle from create snapshot code

I am targeting this branch, because NoficationBundle code wasn't deprecated.

Issues #1415.

Changelog

### Added
- Added `CreateSnapshotService` to create snapshots
- Added `CreateSnapshotBySiteInterface`
- Added `CreateSnapshotByPageInterface`

### Deprecated
- Deprecated `async` mode in `CreateSnapshotsCommand`
- Deprecated `sonata.notification.backend` code into `SiteAdminController`
- Deprecated `sonata.notification.backend` code into `PageAdminController`
- Deprecated `sendMessage` and `$backend` property into the `CreateSnapshotAdminExtension`

To do

Create Snapshot

  • Created test for create snapshot
  • Add BC in create snapshot command
  • Decouple NotificationBundle for create snapshot
  • Create snapshot by command line
  • see where CreateSnapshotAdminExtension is used, and remove notification bundle from there (it was added here: 2365702)
  • Create snapshot, page, and site by admin (it was leading some strange behaviour )
  • Edit doc (remove --site=all, because it's the default value)
  • Fix pipelines
  • validate the same create snapshot behaviour from 3.x branch
  • Add --site=all, just to provide a security execution like "doctrine:schema:update --force"

@eerison eerison changed the title Remove notification bundle Decouple and Deprecate notification bundle Apr 28, 2022
src/Command/BaseCommand.php Outdated Show resolved Hide resolved
src/Command/BaseCommand.php Outdated Show resolved Hide resolved
src/Command/CreateSnapshotsCommand.php Outdated Show resolved Hide resolved
src/Command/CreateSnapshotsCommand.php Outdated Show resolved Hide resolved
src/Command/CreateSnapshotsCommand.php Outdated Show resolved Hide resolved
@eerison eerison force-pushed the remove_notification_bundle branch 2 times, most recently from 1ac060d to 80b5bd3 Compare May 18, 2022 11:56
@eerison
Copy link
Contributor Author

eerison commented May 18, 2022

Note: I'll check all suggestion added in my PR, I didn't forgot this :D

@eerison
Copy link
Contributor Author

eerison commented May 20, 2022

Ok, just a explanation about future remove for this piece of code

https://github.com/sonata-project/SonataPageBundle/blob/3.x/src/Command/CreateSnapshotsCommand.php#L69-L75

I'm trying to coverage the CreateSnapshotsCommand.php before make changes, and I can not coverage this else condition, the reason: when we pass one value or more in --site, always it'll be an array, then never it'll be equals all (and enter into the "else")

maybe in the past it worked different or maybe it get a different behaviour int his PR

just for your information :)

@VincentLanglet
Copy link
Member

maybe in the past it worked different or maybe it get a different behaviour int his PR

Indeed, prior to this PR, this was working.
Now the check would be !\in_array('all', ....)

@eerison
Copy link
Contributor Author

eerison commented May 20, 2022

maybe in the past it worked different or maybe it get a different behaviour int his PR

Indeed, prior to this PR, this was working. Now the check would be !\in_array('all', ....)

But this is not necessary, because if you pass all, it will return all sites and it'll create the snaps shots

it's handled here : https://github.com/sonata-project/SonataPageBundle/blob/3.x/src/Command/BaseCommand.php#L107

Edit 1: it'll work for 1 site, many sites or all we don't need this else to call the same command, and it's working without this "else" ;)

@VincentLanglet
Copy link
Member

Indeed, you might remove the else then. (and the if check)

@eerison eerison force-pushed the remove_notification_bundle branch from 80b5bd3 to 3e42955 Compare June 8, 2022 15:51
@eerison eerison force-pushed the remove_notification_bundle branch 2 times, most recently from 5908777 to a962e8f Compare June 9, 2022 15:31
…ce.php and maybe refactor CreateSnapshotService
@eerison eerison force-pushed the remove_notification_bundle branch from a962e8f to 767dd45 Compare June 9, 2022 15:32
@eerison eerison force-pushed the remove_notification_bundle branch 3 times, most recently from d154eff to 3ee2a12 Compare June 29, 2022 16:50
@eerison eerison force-pushed the remove_notification_bundle branch 2 times, most recently from 452eddd to b014e9d Compare June 29, 2022 19:09
@eerison eerison force-pushed the remove_notification_bundle branch 2 times, most recently from ead67f2 to e2b7095 Compare June 29, 2022 19:31
@eerison eerison force-pushed the remove_notification_bundle branch from e2b7095 to f895cda Compare June 29, 2022 19:42
@eerison eerison force-pushed the remove_notification_bundle branch from ae9b701 to 7de3963 Compare July 6, 2022 13:21
@eerison eerison marked this pull request as ready for review July 6, 2022 13:26
jordisala1991
jordisala1991 previously approved these changes Jul 6, 2022
@VincentLanglet
Copy link
Member

@eerison I added a commit to show you how this could be done, if you want to take a look.
I'm pretty sure I mocked too many things though.

@eerison
Copy link
Contributor Author

eerison commented Jul 7, 2022

@eerison I added a commit to show you how this could be done, if you want to take a look. I'm pretty sure I mocked too many things though.

Ok I'll check ;)

Edit 1: Hey @VincentLanglet I checked your changes and almost the whole code is necessary, I just removed 1 mock and added some NEXT_MAJOR comments. and it's covering the changes that I made in batchActionSnapshot.

@VincentLanglet
Copy link
Member

@eerison I added a commit to show you how this could be done, if you want to take a look. I'm pretty sure I mocked too many things though.

Ok I'll check ;)

Edit 1: Hey @VincentLanglet I checked your changes and almost the whole code is necessary, I just removed 1 mock and added some NEXT_MAJOR comments. and it's covering the changes that I made in batchActionSnapshot.

Then, Am I wrong or is it ready to be merged ?

@VincentLanglet VincentLanglet requested a review from a team July 7, 2022 13:57
@eerison
Copy link
Contributor Author

eerison commented Jul 7, 2022

@eerison I added a commit to show you how this could be done, if you want to take a look. I'm pretty sure I mocked too many things though.

Ok I'll check ;)
Edit 1: Hey @VincentLanglet I checked your changes and almost the whole code is necessary, I just removed 1 mock and added some NEXT_MAJOR comments. and it's covering the changes that I made in batchActionSnapshot.

Then, Am I wrong or is it ready to be merged ?

Yes it's ready to merge ;)

@VincentLanglet
Copy link
Member

Ok, I'll wait for @jordisala1991 review

@jordisala1991 jordisala1991 merged commit 8901345 into sonata-project:3.x Jul 7, 2022
@jordisala1991
Copy link
Member

Thanks @eerison , really good work!

@jordisala1991
Copy link
Member

We missed something here, this other command also uses notificationBundle to work: https://github.com/sonata-project/SonataPageBundle/blob/3.x/src/Command/CleanupSnapshotsCommand.php

@eerison
Copy link
Contributor Author

eerison commented Jul 7, 2022

We missed something here, this other command also uses notificationBundle to work: https://github.com/sonata-project/SonataPageBundle/blob/3.x/src/Command/CleanupSnapshotsCommand.php

Hey @jordisala1991 no I didn't missed this, I will create a PR for this cleanup too, I didn't do this here, because it will be too big

@eerison eerison deleted the remove_notification_bundle branch July 7, 2022 17:46
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.

5 participants