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

Do not overwrite configuration #1155

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

Hlavtox
Copy link
Contributor

@Hlavtox Hlavtox commented Feb 4, 2025

Questions Answers
Description? Adds a bit more resiliency to the upgrade process. If a shop has backported some logic from newer versions, this will prevent the attempts of overwriting the configuration keys, if they are already in the database. I did not check, but it would either fail or insert a duplicate entry.
Type? improvement
BC breaks? no
Deprecations? no
Fixed ticket?
Sponsor company
How to test?

@Hlavtox Hlavtox added this to the 7.0.0 milestone Feb 4, 2025
@Hlavtox Hlavtox force-pushed the do-not-overwrite-configuration branch from a656a86 to 654453d Compare February 4, 2025 13:57
M0rgan01
M0rgan01 previously approved these changes Feb 4, 2025
@Quetzacoalt91 Quetzacoalt91 added the enhancement Type: Improvement label Feb 6, 2025
@ga-devfront
Copy link
Contributor

ga-devfront commented Feb 6, 2025

@Hlavtox I think with new CI merged yesterday you are on error with header. Can you rebase please.

@Hlavtox
Copy link
Contributor Author

Hlavtox commented Feb 6, 2025

@ga-devfront Brother, what do mean by header? It can be merged, there are no conflicts. :-) (I can rebase when I get to my PC of course, no worries.)

@ga-devfront
Copy link
Contributor

@ga-devfront Brother, what do mean by header? It can be merged, there are no conflicts. :-) (I can rebase when I get to my PC of course, no worries.)

We have modified the package that manages header stamps and added it to this project. Even if there is no conflict currently it is very likely that once merged the CI that checks header stamps will end up in error because the header stamps of your PR does not exactly match the new one expected.
I remain at your entire disposal if you have other questions and thank you very much for your contributions.

@Hlavtox Hlavtox force-pushed the do-not-overwrite-configuration branch from 654453d to 78f23c2 Compare February 19, 2025 10:16
@Hlavtox
Copy link
Contributor Author

Hlavtox commented Feb 19, 2025

@ga-devfront @M0rgan01 @Quetzacoalt91 Guys, just rebased it, should be good now. :-)

I remain at your entire disposal if you have other questions and thank you very much for your contributions.

Thank you Guyomar! :-) No worries, making it better makes the process easier for everyone.

('PS_IMAGE_FORMAT', 'jpg', NOW(), NOW())
;
/* PHP:add_configuration_if_not_exists('PS_PRODUCT_REDIRECTION_DEFAULT', '404'); */;
/* PHP:add_configuration_if_not_exists('PS_MAINTENANCE_ALLOW_ADMINS', '1'); */;
Copy link
Member

@Quetzacoalt91 Quetzacoalt91 Feb 19, 2025

Choose a reason for hiding this comment

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

Not sure if this is important, but I see the original value was an integer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will behave the same when inserting it into the database, but I can change it if you want me to.

Copy link
Member

Choose a reason for hiding this comment

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

If you're certain the result will be the same, no need to change it.

ga-devfront
ga-devfront previously approved these changes Feb 19, 2025
Copy link
Contributor

@ga-devfront ga-devfront left a comment

Choose a reason for hiding this comment

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

Thank you for your work and interest !

Quetzacoalt91
Quetzacoalt91 previously approved these changes Feb 19, 2025
@Hlavtox Hlavtox dismissed stale reviews from Quetzacoalt91, ga-devfront, and M0rgan01 via ccfe883 February 19, 2025 10:43
@Hlavtox
Copy link
Contributor Author

Hlavtox commented Feb 19, 2025

Guys, had to change the header, one more approve pls.

BTW, there is something wrong in the header stamping tool of yours. :-))) See:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Ready for review
Development

Successfully merging this pull request may close these issues.

5 participants