-
-
Notifications
You must be signed in to change notification settings - Fork 528
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 deprecated fields from modResource #16187
Remove deprecated fields from modResource #16187
Conversation
Codecov ReportBase: 17.95% // Head: 17.90% // Decreases project coverage by
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## 3.x #16187 +/- ##
============================================
- Coverage 17.95% 17.90% -0.05%
- Complexity 10442 10478 +36
============================================
Files 561 561
Lines 39051 39222 +171
============================================
+ Hits 7010 7024 +14
- Misses 32041 32198 +157
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
I probably have to redo this, I used the GitHub "sync fork" option which did a merge instead of rebase.. |
Do merge commits (* that do not actually include any other commits) still matter if we squash when merging @opengeek? |
They matter because all of those changes become part of this PR. |
Or looking at it, maybe they don't? I have no idea. |
I'd prefer out-of-date changes to be rebased, however. And if there are no conflicts, there is no reason to do this. |
That said, this can be left alone. I'll deal with it when integrating it. |
Ok! The UI of GitHub didn't mention the strategy used for syncing the fork, neither did it ask for confirmation 😅. Next time I'll use the CLI again. |
I've never tried to use that other than on a clean fork. I'll do some more research, but it looks like it did a squash merge, and that's why there are no changes from the merge showing up as part of the PR. If that is the case, there may be no problem doing it that way. |
setup/includes/upgrades/common/3.0.2-remove-deprecated-resource-fields.php
Outdated
Show resolved
Hide resolved
I think you probably did not mean to commit |
@Mark-H yes that’s my mistake, will fix it tomorrow |
b17eab5
to
9ad5f39
Compare
@opengeek if you have time please re-review this PR! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, but can someone confirm it works if the "upgrade" is re-run?
What does it do?
Removes the fields from the schema and drops them from the table.
Why is it needed?
Fields are marked as deprecated and are no longer in use in the core.
How to test
Install/update MODX and see the columns not longer being present in the table.
Related issue(s)/PR(s)
Closes #16174