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

[Maintenance] Deprecate unused configuration nodes #861

Merged
merged 2 commits into from
Sep 26, 2024
Merged

[Maintenance] Deprecate unused configuration nodes #861

merged 2 commits into from
Sep 26, 2024

Conversation

NoResponseMate
Copy link
Contributor

@NoResponseMate NoResponseMate commented Apr 8, 2024 •
edited
Loading

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

From what I see these nodes were never in use.

@NoResponseMate NoResponseMate added the Maintenance Configurations, READMEs, releases, etc. label Apr 8, 2024
@NoResponseMate NoResponseMate requested a review from a team as a code owner April 8, 2024 10:31
diimpp
diimpp reviewed Apr 11, 2024
View reviewed changes
Copy link
Member

@diimpp diimpp left a comment

Choose a reason for hiding this comment

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

Will this change affect existing bundles at 1.13/1.14 sylius?

image

What if some end-user made use of it?

@NoResponseMate
Copy link
Contributor Author

@diimpp
Just rechecked.
There's no problem when it's missing in regards to Sylius, it's not in use per se, and if it's additionally added to Order configuration, for instance, it makes no difference since it's a new node with no connection to the Resource one.
There is an issue though, when someone sets the options in their yaml config; the container will fail to build since the node is no longer there.
No idea why someone would use this node since it's not consumed in any way, but either way just to be safe I'll deprecate it instead.

@NoResponseMate NoResponseMate force-pushed the 1.11-remove-unused-options-node branch from 69d0a3e to bb547c9 Compare April 20, 2024 10:08
@NoResponseMate NoResponseMate changed the title [Maintenance] Remove unused configuration nodes [Maintenance] Deprecate unused configuration nodes Apr 22, 2024
@loic425 loic425 changed the base branch from 1.11 to 1.12 September 4, 2024 07:27
@loic425
Copy link
Member

loic425 commented Sep 4, 2024

@NoResponseMate You missed this doc => https://github.com/Sylius/SyliusResourceBundle/blob/1.12/docs/reference.md

@NoResponseMate NoResponseMate force-pushed the 1.11-remove-unused-options-node branch from bb547c9 to e637d01 Compare September 5, 2024 07:06
@NoResponseMate NoResponseMate force-pushed the 1.11-remove-unused-options-node branch from e637d01 to e73646c Compare September 26, 2024 11:41
@GSadee GSadee merged commit 33ed7b1 into Sylius:1.12 Sep 26, 2024
20 checks passed
@NoResponseMate NoResponseMate deleted the 1.11-remove-unused-options-node branch October 9, 2024 08:43
Prometee pushed a commit to Prometee/SyliusResourceBundle that referenced this pull request Dec 4, 2024
@GSadee
Merge pull request Sylius#861 from NoResponseMate/1.11-remove-unused-…
d87b859 
…options-node

[Maintenance] Deprecate unused configuration nodes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diimpp diimpp diimpp left review comments

@GSadee GSadee GSadee approved these changes

Assignees
No one assigned
Labels
Maintenance Configurations, READMEs, releases, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@NoResponseMate @loic425 @diimpp @GSadee