-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[4.0] Allow empty metakey values in database tables #27854
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
[4.0] Allow empty metakey values in database tables #27854
Conversation
|
@brianteeman Done. I won't change the comments with update SQL scripts because we never do that for comments only. |
|
I have tested this item ✅ successfully on 2723a32 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27854. |
|
Thinking about it ... maybe I should change this to null values instead of empty strings? Is late here, I have to check tomorrow. |
|
I did wonder about that but you're the sql guru |
|
from https://dev.mysql.com/doc/refman/8.0/en/blob.html
|
|
You shall not trust gurus 😝 |
|
I should not have looked into joomla.sql to see how it should be done ... is all wrong there. There is a PR which wants to fix that for staging, #17860 , but it it missing something, and the author seems not to be available anymore. Have pinged him yesterday. Let's wait a bit and give him time. For this PR here I will check if I can provide a correction, and if not I'll close it and make an issue. @Quy Thanks for pointing me to that other PR. Clould you set back your test result to "not tested" for my PR here? |
|
@alikon @brianteeman @Quy PR modified, descriptions and testing instructions updated. Ready for testing. |
|
I have tested this item ✅ successfully on f08bcf4 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27854. |
|
rather urgent to get in as multilingual is dead fish. ;) |
|
I have tested this item ✅ successfully on f08bcf4 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27854. |
|
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27854. |
Pull Request for #25258 (comment).
Summary of Changes
With PR #25258 meta keywords have been removed from diverse forms and views.
This is a good thing.
But the fields in database tables still exist, have a
NOT NULLconstraint and partly no default value, e.g. in the#__languagestable.This leads to content language not being installed when installing a language, see #25258 (comment).
Furthermore I saw fields
metakeyare still used by diverse sample data, so we can't just remove those columns, not yet.So this PR removes the
NOT NULLconstraint frommetakeycolumns where they are of typetext, because for such columns default values are not allowed in strict mode, and adds missing default value empty string to those columns where they are of typeVARCHAR, so things don't fail and stay BC.Later when all metaky removal has been completed we can remove those columns from database tables with another PR, wherever appropriate (BC?).
Testing Instructions
Update
On a current 4.0-dev installation, apply the changes in the update sql script
4.0.0-2020-02-08.sqladded by this PR e.g. with PhpMyAdmin or PhpPgAdmin, replacing#___by your actual db prefix.Install Persian language.
Result: Content language created, no notice like shown in comment [4.0] Remove Meta Keywords (part 1) #25258 (comment).
Verify that following still works:
New installation
Apply PR on a clean current 4.0-dev and then make new installation.
to 3. Same as for update above.
Expected result
Code review/database check: All database columns for meta keywords have default value
''where they are of typeVARCHARand have no default and allowNULLvalues where they are of typeTEXT.Language installation test (step 2): No notice, and content language is created.
Other tests (step 3): Things work.
Actual result
Code review/database check: There are some database columns for meta keywords having a
NOT NULLconstraint and no default value.Language installation test (step 2): Notice shown, and content language not created.
Other tests (step 3): Things work.
Documentation Changes Required
None.