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.

Sign up for GitHub

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

Jump to bottom

[Fixtures] Add a fixture listener for removing images #14095

Merged
merged 2 commits into from
Jun 21, 2022
Merged

[Fixtures] Add a fixture listener for removing images #14095

merged 2 commits into from
Jun 21, 2022

Conversation

GSadee
Copy link
Member

@GSadee GSadee commented Jun 20, 2022

Q A
Branch? 1.11
Bug fix? yes?
New feature? no
BC breaks? no
Deprecations? no
Related tickets
License MIT

Currently without removing pictures when loading fixtures, the images directory can grow unnecessarily very quickly.

@GSadee GSadee added Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). DX Issues and PRs aimed at improving Developer eXperience. labels Jun 20, 2022
@GSadee GSadee requested a review from a team as a code owner June 20, 2022 06:07
Zales0123
Zales0123 reviewed Jun 20, 2022
View reviewed changes
Copy link
Member

@Zales0123 Zales0123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 but the problem is we already have a .gitkeep file placed in public/media/image/ directory, which will also be removed after running the fixtures and therefore would result in changed in the repository 💃 Maybe it would be better to remove all the things (files and catalogues) from the inside of images directory omitting the .gitkeep file? 🤔

@GSadee GSadee force-pushed the removing-images-by-fixture-listener branch from 0625ab7 to fa75b00 Compare June 20, 2022 08:34
@GSadee GSadee changed the title Add a fixture listener for removing images [Fixtures] Add a fixture listener for removing images Jun 20, 2022
@GSadee GSadee force-pushed the removing-images-by-fixture-listener branch from fa75b00 to cd238b4 Compare June 20, 2022 08:37
@GSadee
Copy link
Member Author

GSadee commented Jun 20, 2022

👍 but the problem is we already have a .gitkeep file placed in public/media/image/ directory, which will also be removed after running the fixtures and therefore would result in changed in the repository 💃 Maybe it would be better to remove all the things (files and catalogues) from the inside of images directory omitting the .gitkeep file? 🤔

I've refactored this code by removing all files from the given directory and creating the .gitkeep file after that, as it seems to be the simplest solution to resolve the issue you mentioned, wdyt?

Zales0123
Zales0123 approved these changes Jun 20, 2022
View reviewed changes
Copy link
Member

@Zales0123 Zales0123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I wonder now couldn't it be named FilesPurgerListener and land in the SyliusFixturesBundle itself 🚀 It would then be possible to use it e.g. for invoices removal with SyliusInvoicingPlugin

@AdamKasp AdamKasp merged commit 83d414d into Sylius:1.11 Jun 21, 2022
@AdamKasp
Copy link
Contributor

Thanks, Grzegorz! 🎉

@GSadee GSadee deleted the removing-images-by-fixture-listener branch June 21, 2022 08:06
@GSadee GSadee mentioned this pull request Jun 22, 2022
Merged
AdamKasp added a commit that referenced this pull request Jun 23, 2022
@AdamKasp
bug #14102 [Fixtures] Fix removing images before suite (GSadee)
c994b72 
This PR was merged into the 1.11 branch.

Discussion
----------

| Q               | A                                                            |
|-----------------|--------------------------------------------------------------|
| Branch?         | 1.11|
| Bug fix?        | yes                                                       |
| New feature?    | no                                                      |
| BC breaks?      | no                                                      |
| Deprecations?   | no|
| Related tickets | fixes #14095                       |
| License         | MIT                                                          |

<!--
 - Bug fixes must be submitted against the 1.10 or 1.11 branch(the lowest possible)
 - Features and deprecations must be submitted against the master branch
 - Make sure that the correct base branch is set

 To be sure you are not breaking any Backward Compatibilities, check the documentation:
 https://docs.sylius.com/en/latest/book/organization/backward-compatibility-promise.html
-->


Commits
-------

40e3edb [Fixtures] Fix removing images before suite
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rafikooo Rafikooo Rafikooo approved these changes

@AdamKasp AdamKasp AdamKasp approved these changes

@ernestWarwas ernestWarwas ernestWarwas approved these changes

@Zales0123 Zales0123 Zales0123 approved these changes

Assignees
No one assigned
Labels
DX Issues and PRs aimed at improving Developer eXperience. Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@GSadee @AdamKasp @Zales0123 @ernestWarwas @Rafikooo