Skip to content
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

SQL - improve CAST with CAST('field' AS CHAR(n)) #12417

Merged
merged 11 commits into from
Mar 30, 2017
Merged

SQL - improve CAST with CAST('field' AS CHAR(n)) #12417

merged 11 commits into from
Mar 30, 2017

Conversation

alikon
Copy link
Contributor

@alikon alikon commented Oct 15, 2016

as per comment on #12348 see #12348 (review)

Summary of Changes

  • implemented missing feature for SQL CAST CAST('field' AS CHAR(n))
  • moved the castAsChar() from query.php to each driver

Testing Instructions

this kind of CAST works now
$query->select($query->castAsChar('field', 40))
$query->select($query->castAsChar('field'))

Documentation Changes Required

we can use this type of SQL too
$query->select($query->castAsChar('field', 40))

SQL - improve CAST('field' AS CHAR(n))
SQL - improve CAST('field' AS CHAR(n))
moved castAsChar to the driver
CAST with CAST('field' AS CHAR(n))
tab
tabs again
@mbabker
Copy link
Contributor

mbabker commented Oct 15, 2016

You need to keep the base function in JDatabaseQuery otherwise if it's used with a subclass not implementing the method you'll end up with a fatal error. Otherwise aside from the CI failures seems fine to me.

re-added castAsChar() as per comment
@alikon
Copy link
Contributor Author

alikon commented Oct 16, 2016

restored castAsChar() in JDatabaseQuery as suggested

@waader
Copy link
Contributor

waader commented Dec 27, 2016

I have tested this item ✅ successfully on c5711ba

Tested with mysql and postgresql. Thanks!


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

@jeckodevelopment
Copy link
Member

@alikon Nicola, can you please check the conflicting file?
libraries/joomla/database/query/mysqli.php

@alikon
Copy link
Contributor Author

alikon commented Dec 28, 2016

conflict fixed

@waader
Copy link
Contributor

waader commented Feb 26, 2017

The mssql implementation is missing here.

the mssql castAsChar()
@alikon
Copy link
Contributor Author

alikon commented Feb 27, 2017

@waader added the mssql part

@csthomas
Copy link
Contributor

csthomas commented Mar 2, 2017

Can you change default len for mssql from 10 to 30.

https://msdn.microsoft.com/en-us/library/ms187928.aspx

The default value is 30.

It would be useful for datetime2 which has 19 chars. 0000-00-00 00:00:00

@waader
Copy link
Contributor

waader commented Mar 28, 2017

@alikon: is this ready for testing?

msdn
@alikon
Copy link
Contributor Author

alikon commented Mar 28, 2017

yes now fixed last comment from @csthomas about mssql

@waader
Copy link
Contributor

waader commented Mar 28, 2017

I have tested this item ✅ successfully on a3bad12

Thanks!


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

@csthomas
Copy link
Contributor

I have tested this item ✅ successfully on a3bad12


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

@jeckodevelopment
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Mar 29, 2017
@jeckodevelopment jeckodevelopment added this to the Joomla 3.7.0 milestone Mar 29, 2017
@rdeutz rdeutz merged commit 5d4539a into joomla:staging Mar 30, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Mar 30, 2017
@alikon alikon deleted the patch-84 branch March 31, 2017 05:59
@wilsonge
Copy link
Contributor

wilsonge commented May 7, 2019

@alikon Would you mind porting this to the framework package so that it can go into the 4.x branch please - I just realised it's gone missing (issue to track this on framework at joomla-framework/database#157)

@alikon
Copy link
Contributor Author

alikon commented May 7, 2019

went out of my radar i'll try to add #18961 then too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants