-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Change back data type of template styles home column in db #25484
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
Change back data type of template styles home column in db #25484
Conversation
|
For the record, the fact you have to rename the index is exactly why the bug in the schema parser needs to be fixed. You should not have to version suffix table indexes. |
|
@mbabker Yes, I know. But it is not a bug, it is missing by design. That's why I did not fix it yet, it is a bigger thing. |
|
Attention, PR is not ready yet, changes in joomla.sql are missing. But you can already apply the PR and run the DB fixer for testing the schema updates are working. |
|
It's a bug if it's missing this critical of a feature 😉 (And yeah, I realize fixing the database code is a lot more effort than it's sometimes worth, but still have to make it known I hate hacky fixes to work around a deeper underlying problem) |
|
ALTER TABLE |
|
@mbabker I absolutely agree with you about the issue, but it is not just a bug fix. It needs a redesign of the schema checker. That's why we now have to use the ugly hacks, we don't have the time now to fix the schema checker. |
|
@HLeithner Not sure. For postgres I have used "IF EXISTS", that should be ok. For the other DB types we don't have that yet. I have to check that. |
|
Hint for testers: Link to modified update package in description has changed, package has been updated by latest changes on this PR. |
Something like this for MySQL can help? |
|
@simbus82 That would work for the installer when doing the update, but the database schema checker would be confused. we found other way. pr is ready for being tested. |
|
@alikon @twister65 Can you test this PR for postgres? I gotta go sleep now. |
|
I have tested this item ✅ successfully on 71252b7 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25484. |
|
I have tested this item ✅ successfully on 71252b7 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25484. |
|
Status "Ready To Commit". |
|
@alikon not good enough for a postgresql test for this I'm afraid. We need to be sure our index's are being preserved. Anyone else here able to do a postgresql test before this is merged |
|
Set back on pending by comment of @wilsonge above. |
|
i know, but cannot help today more than this ... |
|
https://docs.joomla.org/Tables/template_styles i've updated this page to state it's a varchar and noted the type of the database |
|
@alikon are you able to at least test it with the index's applied to that table rather than just with the table structure? I installed postgres with brew last night onto my macbook but didn't get time to set up a full joomla install - and won't be able to until tonight |
|
sorry @wilsonge & all, i'm able to do only some spot SQL Fiddle |
|
I tested all postgresql scenarios with postgresql 9.6 and worked now. thx richard |
Pull Request for Issue #25482 .
Summary of Changes
Change back data type of column
hometo string (type depending on db type) in table#__template_styles, because it can not only hold values '0' or '1' but also language code, e.g. 'de-DE', if the template style is a default for a particular language.This is an undo of PR #24595 .
Testing Instructions
4 different tests.
Update Joomla 3.9.8 site to a patched 3.9.9 package with this PR and the version number patched to 3.10. PLEASE DO THIS ONLY ON TESTING SITE, NOT ON LIFE SITE.
You can find such a package here:
https://test5.richard-fath.de/Joomla_3.9.10-dev-Update_Package_pr-25484.zip.
Do the same with a 3.9.9: Update it to my patched package.
On a 3.9.8, apply this PR e.g. with patch tester, then go to "Extensions -> Manage -> Database". Use the "Fix" button to apply the changes in the new schema updates from this PR.
Do the same as in step 3 but on a 3.9.9
Expected result
In all cases the update or schema fix succeeds, and issue #25482 is solved.
Actual result
See issue #25482 .
Documentation Changes Required
None.