? Success

User tests: Successful: Unsuccessful:

avatar photodude
photodude
17 May 2016

Pull Request for Issue where some mysql unsigned numeric types will throw table ___ does not have column ___ with type ___ in the Extensions: Database update checks.

Basically, the fixInteger() function makes an overly broad assumption that any type passed in type1 either needs to be returned as that type or if it's the full word type integer that it should check if the type2 is unsigned.

In some cases unsigned numeric types were unaffected, but for select cases fixInteger() method actually resulted in stripping out the unsigned designation for those numeric types during database updates.

Summary of Changes

Fixes all cases where the mysql numeric types are unsigned.

Prior to this patch, table changes to add unsigned to some numeric types would result in the the database check trowing an error table ___ does not have column ___ with type ___ This typically happens with things like int(10)

With this patch all mysql numeric types retain the unsigned designation.

Testing Instructions

https://docs.joomla.org/Testing_Joomla!_patches

  • Use this sample data schema change to get a database schema with unsigned types 3.6.0-2016-06-20.zip
    • place the sample data schema change file at administrator/components/com_admin/sql/updates/mysql/
    • (or create your own schema change with unsigned numeric types other than integer)
  • go to Extensions: Database and press the fix button
    • note: that some columns will continue to say table ___ does not have column ___ with type int(10)
    • note: although these columns say int(10) none say unsigned
  • Now apply this patch
  • go to Extensions: Database
    • note: there should now be an additional notices for columns with int(11) and bigint(20) and all types stating unsigned
  • press the fix button and you should get the notice Database table structure is up to date. for a successful test
avatar photodude photodude - open - 17 May 2016
avatar photodude photodude - change - 17 May 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 17 May 2016
Labels Added: ?
avatar brianteeman brianteeman - change - 17 May 2016
Category SQL
avatar photodude photodude - change - 19 May 2016
Title
Fix for int(11) unsigned or bigint unsigned, etc in mysql schema
Fix for some unsigned numeric types in mysql schema
avatar photodude photodude - change - 19 May 2016
Title
Fix for int(11) unsigned or bigint unsigned, etc in mysql schema
Fix for some unsigned numeric types in mysql schema
avatar photodude photodude - change - 19 May 2016
The description was changed
avatar photodude photodude - change - 19 May 2016
The description was changed
avatar mbabker mbabker - reference | 0ea95b1 - 23 May 16
avatar photodude
photodude - comment - 11 Jun 2016

@mbabker @wilsonge outside of the google mailing list is there another way to get testers for this PR? Or can this be merged by code review?

avatar wojsmol
wojsmol - comment - 18 Jun 2016

@photodude testing now ?

avatar wojsmol wojsmol - test_item - 18 Jun 2016 - Tested unsuccessfully
avatar wojsmol
wojsmol - comment - 18 Jun 2016

I have tested this item ? unsuccessfully on 78798b2

I get

1062 Duplicate entry '19-0' for key 'PRIMARY' SQL=ALTER TABLE `#__modules_menu` MODIFY `menuid` int(11) UNSIGNED NOT NULL DEFAULT '0';

on

Database Version    5.5.28 
PHP Version     5.4.45


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

avatar photodude
photodude - comment - 18 Jun 2016

@wojsmol what version of Joomla? That looks like it's coming from an unrelated change.
Please also include all of the messages from the database fixer (like which SQL file it's not working with)

avatar wojsmol
wojsmol - comment - 18 Jun 2016

@photodude staging dowloaded after #10538 (comment)

avatar photodude
photodude - comment - 18 Jun 2016

Please try checking the staging branch without this patch and verify that the error doesn't show up.

A number of changes happened between 3.5.1 when I wrote this and the current staging. This patch should not affect the record adds which would be resulting in the duplicate key.

avatar wojsmol
wojsmol - comment - 18 Jun 2016

@photodude
2 cases:

  • The lastest staging with testing sample data + #10098 + this PR - see #10538 (comment)
  • The lastest staging without sample data + #10098 this PR - success

additional note

Database Collation  utf8_general_ci
Database Connection Collation   utf8mb4_general_ci 
avatar photodude
photodude - comment - 20 Jun 2016

@wojsmol in your testing did you get a message like `Table '_______' does not have column '_____' with type int(10). (From file 3.6.0-2016-06-01.sql.) ?

I need those messages to have a better idea of where the duplicate key is coming from and if it's related to this PR, the other PR or something else.

If I remember right Joomla 3.0+ has the option to choose which type of sample data to install:
So which Sample Data Set did you use?:

Blog English (GB) Sample Data
Brochure English (GB) Sample Data
Default English (GB) Sample Data
Learn Joomla English (GB) Sample Data
Test English (GB) Sample Data

avatar photodude
photodude - comment - 21 Jun 2016

@wojsmol I can not replicate the issue that caused your test to fail with either 3.5.1, 3.6.0.beta.2 or with the current staging. As far as I can tell it's likely something with your test environment. Can you try a completely fresh install from 3.6.0.beta.2 or current staging?

avatar wojsmol
wojsmol - comment - 21 Jun 2016

@photodude I retested now case with sampledata using following test procedure -
after each step checking whether the structure of the database is correct.

  1. Install staging on commit 9e596d1 with Test English (GB) Sample Data

    Database table structure is up to date.

  2. install patch tester v 2.0.1

    Database table structure is up to date.

  3. Applay #10098 on commit f7c2ea3

    Table '#__contact_details' does not have column 'access' with type int(10). (From file 3.6.0-2016-05-16.sql.)
    Table '#__finder_links' does not have column 'access' with type int(10). (From file 3.6.0-2016-05-16.sql.)
    Table '#__finder_taxonomy' does not have column 'access' with type int(10). (From file 3.6.0-2016-05-16.sql.)
    Table '#__newsfeeds' does not have column 'id' with type int(10). (From file 3.6.0-2016-05-16.sql.)
    Table '#__newsfeeds' does not have column 'access' with type int(10). (From file 3.6.0-2016-05-16.sql.)
    
  4. Click fix button

    Table '#__contact_details' does not have column 'access' with type int(10). (From file 3.6.0-2016-05-16.sql.)
    Table '#__finder_links' does not have column 'access' with type int(10). (From file 3.6.0-2016-05-16.sql.)
    Table '#__finder_taxonomy' does not have column 'access' with type int(10). (From file 3.6.0-2016-05-16.sql.)
    Table '#__newsfeeds' does not have column 'id' with type int(10). (From file 3.6.0-2016-05-16.sql.)
    Table '#__newsfeeds' does not have column 'access' with type int(10). (From file 3.6.0-2016-05-16.sql.)
    
  5. Applay #10538 on commit 78798b2

    Table '#__banners' does not have column 'id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__banners' does not have column 'cid' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__banner_clients' does not have column 'id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__categories' does not have column 'id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__contact_details' does not have column 'id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__contact_details' does not have column 'user_id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__contact_details' does not have column 'catid' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__content_frontpage' does not have column 'content_id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__content_rating' does not have column 'content_id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__contentitem_tag_map' does not have column 'content_item_id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__extensions' does not have column 'extension_id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__menu' does not have column 'id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__modules' does not have column 'id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__modules_menu' does not have column 'moduleid' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__modules_menu' does not have column 'menuid' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__newsfeeds' does not have column 'catid' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__overrider' does not have column 'id' with type int(10) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__postinstall_messages' does not have column 'extension_id' with type bigint(20) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__schemas' does not have column 'extension_id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__session' does not have column 'userid' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__ucm_base' does not have column 'ucm_item_id' with type int(10) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__ucm_base' does not have column 'ucm_type_id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__updates' does not have column 'update_id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__updates' does not have column 'update_site_id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__updates' does not have column 'extension_id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__update_sites' does not have column 'update_site_id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__update_sites_extensions' does not have column 'update_site_id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__update_sites_extensions' does not have column 'extension_id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__users' does not have column 'id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__user_profiles' does not have column 'user_id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    
  6. Click fix button

    An error has occurred.
    1062 Duplicate entry '19-0' for key 'PRIMARY' SQL=ALTER TABLE #__modules_menu MODIFY menuid int(11) UNSIGNED NOT NULL DEFAULT '0';

  7. Check database status one more time

    Table '#__modules_menu' does not have column 'menuid' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__newsfeeds' does not have column 'catid' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__overrider' does not have column 'id' with type int(10) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__postinstall_messages' does not have column 'extension_id' with type bigint(20) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__schemas' does not have column 'extension_id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__session' does not have column 'userid' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__ucm_base' does not have column 'ucm_item_id' with type int(10) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__ucm_base' does not have column 'ucm_type_id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__updates' does not have column 'update_id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__updates' does not have column 'update_site_id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__updates' does not have column 'extension_id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__update_sites' does not have column 'update_site_id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__update_sites_extensions' does not have column 'update_site_id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__update_sites_extensions' does not have column 'extension_id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__users' does not have column 'id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    Table '#__user_profiles' does not have column 'user_id' with type int(11) unsigned. (From file 3.6.0-2016-05-16.sql.)
    
avatar photodude
photodude - comment - 21 Jun 2016

Thanks @wojsmol for the additional information. Everything looks as I expect things to look, except for the Duplicate entry error. From the database status you can see what this patch is correcting. Maybe something in #10098 or Maybe #10098 needs to address something in the sample data.

Try applying this patch #10538 before #10098

@mbabker Do you have any thoughts on what might be causing the Duplicate entry error only when the sample data is included?

avatar photodude
photodude - comment - 21 Jun 2016

@wojsmol I'm almost certain the issue is with either the sample data or with #10098. I've added a test file with a smaller group that should allow testing of this PR without interference from that duplicate entry error.

3.6.0-2016-06-20.zip

avatar wojsmol
wojsmol - comment - 21 Jun 2016

@photodude When testing with sql file from #10538 (comment) the test is successful.

avatar wojsmol
wojsmol - comment - 21 Jun 2016

@brianteeman Please alter my test result.

avatar photodude
photodude - comment - 21 Jun 2016

Thanks @wojsmol. Would you report your failed test under #10098 so we can keep that conversation going over there?

I'll alter the test instructions here as well.

avatar photodude
photodude - comment - 24 Jun 2016

@wojsmol Testing #10538 with #10098 using sample data should work now.

avatar photodude
photodude - comment - 2 Jul 2016

@alex7r would you test this PR so we can get it to RTC?

avatar alex7r
alex7r - comment - 2 Jul 2016

@photodude, I'll test ASAP, 1-2 days.

avatar alex7r alex7r - test_item - 5 Jul 2016 - Tested successfully
avatar alex7r
alex7r - comment - 5 Jul 2016

@photodude I have tested this item successfully on 78798b2

Tested successfully with signed and unsigned int, tinyint, both with different length.


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

avatar photodude
photodude - comment - 5 Jul 2016

@wilsonge @brianteeman can this be marked RTC with the two successful tests. Would be nice to see this in 3.6.0 if possible or at least 3.6.1.

avatar brianteeman brianteeman - alter_testresult - 5 Jul 2016 - Wojsmol: Tested successfully
avatar brianteeman brianteeman - change - 5 Jul 2016
Status Pending Ready to Commit
avatar brianteeman
brianteeman - comment - 5 Jul 2016

Rtc

Sorry didn't see the request to change a test resukt


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

avatar joomla-cms-bot joomla-cms-bot - change - 5 Jul 2016
Labels Added: ?
avatar photodude
photodude - comment - 5 Jul 2016

Thanks everyone.

avatar brianteeman brianteeman - change - 12 Jul 2016
Milestone Added:
avatar roland-d
roland-d - comment - 16 Jul 2016

@photodude You are changing int() to bigint() in #10098 so should the fixInteger() not use bigint as well?

avatar alex7r
alex7r - comment - 16 Jul 2016

@roland-d This PR just fix existing checks and it's don't include any other check, for wrong SQL for example.

avatar roland-d
roland-d - comment - 16 Jul 2016

@alex7r Yes I can see that but what is the use of adding int when in another PR we change everything again to bigint. Just making sure we are not doing duplicate work.

avatar alex7r
alex7r - comment - 16 Jul 2016

@roland-d well it's not about changing something to something, it's about allowing another option. Use case:
SQL contains field with int(8) unsigned.
this will never be fixed by Joomla + passing SQL integrity checks, as check of type is generated wrong.

avatar roland-d
roland-d - comment - 16 Jul 2016

@alex7r I am merely trying to understand what is going on here, so I appreciate your input.

When I have a field like test INT(8) UNSIGNED this is wrong?

avatar alex7r
alex7r - comment - 16 Jul 2016

@roland-d I'm at PC now so can explain it fully I think, here:
There is nothing wrong having field like test INT(8) UNSIGNED,
But
Joomla's SchemaItem generates wrong check for it,
so Joomla thinks this field in your DB has wrong type and throws alert.
If
you'll try to fix this with Joomla, or manually trigger alter sql
your field still will have same type and Joomla still will complain about this,
because
check generated is wrong.
This PR
fixes generation of check sql, so Joomla treat it right.

avatar roland-d
roland-d - comment - 16 Jul 2016

Thank you for the explanation @alex7r

avatar roland-d roland-d - reference | ab5e23d - 16 Jul 16
avatar photodude photodude - close - 16 Jul 2016
avatar roland-d roland-d - merge - 16 Jul 2016
avatar roland-d roland-d - close - 16 Jul 2016
avatar roland-d roland-d - change - 16 Jul 2016
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2016-07-16 12:17:00
Closed_By roland-d
avatar roland-d roland-d - close - 16 Jul 2016
avatar roland-d roland-d - merge - 16 Jul 2016
avatar joomla-cms-bot joomla-cms-bot - close - 16 Jul 2016
avatar joomla-cms-bot joomla-cms-bot - change - 16 Jul 2016
Labels Removed: ?
avatar photodude photodude - change - 16 Jul 2016
The description was changed
avatar photodude
photodude - comment - 16 Jul 2016

@alex7r Thanks for taking the time to fill in that additional explanation.

@roland-d it's basically as @alex7r said. I would point out from the PR description. "Basically, the fixInteger() function makes an overly broad assumption that any type passed in type1 either needs to be returned as that type or if it's the full word type integer that it should check if the type2 is unsigned."

So prior to this change fixInteger() was just enforcing the full word type integer to be int(10) and unsigned if designated unsigned; this is partly due to how mysql versions treat the word full word type integer. This fix broadened that function, as @alex7r pointed out, to ensure that Joomla's SchemaItem didn't ignore cases where fixInteger()'s parameter type2 is unsigned.

Glad to see this is merged. Hopefully we can now work through getting some better consistency in the database scheme as noted in #10077 and partially addressed in #10098

avatar photodude photodude - head_ref_deleted - 16 Jul 2016
avatar roland-d
roland-d - comment - 16 Jul 2016

@photodude Thank you as well for explaining this further. It is not that I have anything against this PR but as mentioned before I was just trying to understand what it was doing. Now it is all clear to me ?

Add a Comment

Login with GitHub to post a comment