Pending

User tests: Successful: Unsuccessful:

avatar frankmayer
frankmayer
24 Jan 2017

Summary of Changes

Fixed two issues with the initial com_fields sql update file for MySQL (I don't have PostgreSQL or MSSQL, so someone should probably look at possible constraints of those, too).

  1. Due to the length of the context field, combined with the utf8mb4 unicode default for the table, it will not create the index. That is, because it would surpass the max length of 767 bytes for MySQL.
    I changed the specific fields to utf8, so the max length of 767 bytes is not surpassed.

  2. TEXT/BLOBS cannot have a default
    Removed defaults for those in this specific file.

Testing Instructions

Run the sql file before the patch, notice the errors.
Run the sql file after the patch, notice the errors missing.

Documentation Changes Required

The issue with the max key length (for mysql and maybe others), should probably be noted in the docs.

avatar frankmayer frankmayer - open - 24 Jan 2017
avatar frankmayer frankmayer - change - 24 Jan 2017
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 24 Jan 2017
Category SQL Administration com_admin
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 24 Jan 2017

@frankmayer how to "run sql file" administrator/components/com_admin/sql/updates/mysql/3.7.0-2016-08-29.sql?

avatar frankmayer
frankmayer - comment - 24 Jan 2017

@franz-wohlkoenig I created a new empty DB and ran it. It will create #__ tables, but that is not a problem for this test.
The other way to test it, would be to have a status of an installation prior to that date and then do a fix on the database.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 24 Jan 2017

@frankmayer how to set the status of the installation prior to 2016-08-29?

avatar frankmayer
frankmayer - comment - 24 Jan 2017

@franz-wohlkoenig hmm... that depends... if you're under git in that testing environment, you might be able to check out the state, prior to that specific commit. If the testing environment is not under git, then you could again check out that state and copy the files over to a previously installed 3.6 setup...

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 24 Jan 2017

@frankmayer Thanks for information, but i don't understand, what to do. Hope two other do.

avatar frankmayer
frankmayer - comment - 24 Jan 2017

@franz-wohlkoenig thanks for looking into it. Maybe I can help you:

  1. Do you have one or more test environments?
  2. What is the setup?
  3. Can you create a new test environment? (install Joomla 3.5 and the patch tester extension)
    (If you can create a new one, I would make a backup of the db and the files, directly after the patch tester installation, for later restoring, if needed)
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 24 Jan 2017

@frankmayer thanks, that i can try. Have a test environment.

avatar frankmayer
frankmayer - comment - 24 Jan 2017

@franz-wohlkoenig Sorry, I made a mistake. You can install Joomla 3.6 instead of 3.5.

Then:

  1. download the zip file of the commit where (Download or Clone - button) here https://github.com/joomla/joomla-cms/tree/2570285679ddd9174694687d2dca87f1f2581c04
    (This is the state of the files in the commit, where this file was introduced.
  2. Unzip the file into the new test environment, overwriting older files.(Be careful with the paths)
  3. Then goto Extensions->Manage->Database. It should say that it's not up to date. hit fix. That should (theoretcally) do the job
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 24 Jan 2017

@frankmayer Can i use 3.7-beta1, not 3.6 - would be easier.

avatar Bakual
Bakual - comment - 24 Jan 2017

We use utf8mb4 for security reasons. Changign that back is probably not an option. Certainly has to be verified by JSST.

avatar mbabker
mbabker - comment - 24 Jan 2017

It does have a security benefit but that's not the only reason (otherwise we would have dropped support for all MySQL versions below 5.5.3 and not bothered supporting utf8/utf8mb4 conversion in the code).

That said though, there does need to be valid reason to force a field to not support utf8mb4. Index length isn't one, we have other places in the schema where length issues are dealt with without changing the field type.

avatar frankmayer
frankmayer - comment - 24 Jan 2017

@Bakual thanks, didn't know that. However, would that be a problem with the context?
@mbabker IK, I understand. However, in some cases an index might be a valid reason for a change (for example in combined indexes) or if there really must be an index for performance reasons. (which I would consider this one to be)

avatar mbabker
mbabker - comment - 24 Jan 2017

If the field will never have the possibility of storing data requiring the utf8mb4 supported character range then it might be acceptable.

But now I'm going to ask. What is this field actually used for, what's being stored in the field to begin with, and what's causing the field to exceed the 191 character length that is usually where utf8mb4 cuts off but is still valid for 255 characters?

avatar frankmayer
frankmayer - comment - 24 Jan 2017

@mbabker You are right.. under normal circumstances the context would never get that high. Would you then suggest to keep it under that 191 chars, instead?

avatar mbabker
mbabker - comment - 24 Jan 2017

Guess it depends what's in the field. If it's not going to get that long then shortening it is fine. I just have no idea what data is in any of these fields tables so I'd be guessing.

avatar frankmayer
frankmayer - comment - 24 Jan 2017

Oh, it's the context like com_content.article

avatar Bakual
Bakual - comment - 24 Jan 2017

The context comes directly from the model. It's the context that is passed to the plugin events. I would be very surprised if that one ever gets bigger than 191 chars. If it does, the extension developer certainly messed something up.

avatar alikon
alikon - comment - 24 Jan 2017

others jtables fields don't need to have this limitations/constraints like 'alias' maybe

avatar laoneo
laoneo - comment - 25 Jan 2017

You can build a full blown CCK on top of com_fields where the context is not representing an simple entity but more of a content type. Then I can imagine that the context can become bigger. But till now I guess it wont.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 22 Feb 2017

looks like a Production Department Decision.

avatar laoneo
laoneo - comment - 11 Apr 2017

Just for the record, the context index issue is solved by #14626.

avatar frankmayer
frankmayer - comment - 1 Jun 2017

Don't know if it makes any sense to keep this PR open, now that 3.7.0 is released. Thoughts?

avatar alikon
alikon - comment - 3 Jun 2017

please close it

avatar franz-wohlkoenig franz-wohlkoenig - change - 3 Jun 2017
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2017-06-03 17:01:30
Closed_By franz-wohlkoenig
avatar joomla-cms-bot joomla-cms-bot - edited - 3 Jun 2017
avatar joomla-cms-bot joomla-cms-bot - close - 3 Jun 2017
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 3 Jun 2017

closed as stated above.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13730.

avatar joomla-cms-bot
joomla-cms-bot - comment - 3 Jun 2017

Add a Comment

Login with GitHub to post a comment