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

[com_fields] MSSQL - sql for Custom fields #12660

Merged
merged 21 commits into from
Jan 7, 2017
Merged

[com_fields] MSSQL - sql for Custom fields #12660

merged 21 commits into from
Jan 7, 2017

Conversation

alikon
Copy link
Contributor

@alikon alikon commented Oct 31, 2016

Pull Request for Issue #12624 .

Summary of Changes

MSSQL - update sql for Custom fields (#11833)
@zero-24
Copy link
Contributor

zero-24 commented Oct 31, 2016

@alikon this just includes the update part is that correct / expected?

Also this is all in one line? is this expected too?

@alikon
Copy link
Contributor Author

alikon commented Oct 31, 2016

No, I'm having big connection issues, hope to solve soon i'll put install
file too, sorry...

On 31 Oct 2016 3:15 pm, "zero-24" [email protected] wrote:

@alikon https://github.com/alikon this jjust includes the update part
is that correct / expected?

Also this is all in one line? is this expected too?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#12660 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AALFseYWgpypxgDH7RwRdfDNjlLpPyDyks5q5fgQgaJpZM4KlDXf
.

@zero-24
Copy link
Contributor

zero-24 commented Oct 31, 2016

no problem. Thanks!

@andrepereiradasilva
Copy link
Contributor

@alikon can you please add line endings

@alikon
Copy link
Contributor Author

alikon commented Oct 31, 2016

@andrepereiradasilva I'll complete this pr when I regain a stable
connection ;)

On 31 Oct 2016 3:57 pm, "andrepereiradasilva" [email protected]
wrote:

@alikon https://github.com/alikon can you please add line endings


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#12660 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AALFsVWAyEckp3pUJLs7zA2hYZ04JP4wks5q5f2vgaJpZM4KlDXf
.

MSSQL - update sql for Custom fields (#11833)
MSSQL - install sql for Custom fields (#11833)
@alikon
Copy link
Contributor Author

alikon commented Oct 31, 2016

can someone do a review or a test on MSSQL (cause i don't have a mssql environment available)

@laoneo
Copy link
Member

laoneo commented Nov 1, 2016

Had a discussion with @matrikular about the MSSQL scripts. Perhaps he can test it as well.

removed (version, hits) fields #12674
removed (version,hits) fields #12674
Fixing sql fields #13091
mssql com_fields#13091
@alikon
Copy link
Contributor Author

alikon commented Dec 5, 2016

updated for #13091

@alikon alikon changed the title MSSQL - update sql for Custom fields (#11833) MSSQL - sql for Custom fields Dec 5, 2016
@laoneo
Copy link
Member

laoneo commented Dec 6, 2016

Can you add the "[com_fields]" to the beginning of the title? People start asking about open pull requests and I'm telling them that they have to search for com_fields. Thanks.

@Bakual Bakual changed the title MSSQL - sql for Custom fields [com_fields] MSSQL - sql for Custom fields Dec 6, 2016
@Bakual
Copy link
Contributor

Bakual commented Dec 6, 2016

Adjusted the title

[com_fields] No need for an alias in fields groups. #13115
[com_fields] No need for an alias in fields groups. #13115
@alikon
Copy link
Contributor Author

alikon commented Dec 21, 2016

updated for #13246

@csthomas
Copy link
Contributor

csthomas commented Jan 4, 2017

I have tested it on backend and it is OK.

Why there is a few difference between sql install and update file?

joomla_sql_diff

@zero-24
Copy link
Contributor

zero-24 commented Jan 5, 2017

The install and update file need to be equal as we need everything (tables etc.) also on sites that got updated to 3.7.0 :)

added the missed DEFAULT
@alikon
Copy link
Contributor Author

alikon commented Jan 5, 2017

fixed the missed default

@csthomas
Copy link
Contributor

csthomas commented Jan 5, 2017

After I patched by Patch Tester a see (it is incorrect - table name should not contain column name):

Table 'j37_fields( id' does not exist. (From file 3.7.0-2016-08-29.sql.)
Table 'j37_fields_categories(  field_id' does not exist. (From file 3.7.0-2016-08-29.sql.)
Table 'j37_fields_groups( id' does not exist. (From file 3.7.0-2016-08-29.sql.)
Table 'j37_fields_values( field_id' does not exist. (From file 3.7.0-2016-08-29.sql.)

After I click FIX the tables are created and fields work OK.

But I still see above warnings.

Please add white space before '(':

CREATE TABLE [#__fields](

replace to:

CREATE TABLE [#__fields] (

and for others "CREATE TABLE" do the same.


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

alikon added 2 commits January 5, 2017 19:58
added space before (
added space before (
@alikon
Copy link
Contributor Author

alikon commented Jan 5, 2017

@csthomas added the space, but all other existing CREATE TABLE don't have that space
anyway thanks for testing

@csthomas
Copy link
Contributor

csthomas commented Jan 5, 2017

I have tested this item ✅ successfully on 6fad1a2

Because of some regex in component installer for database fix "CREATE TABLE" requires space.
It is only needed in update sql files:)

Success.

Database schema version (in #__schemas): 3.7.0-2016-11-24.
Update version (in #__extensions): 3.7.0-alpha2.
Database driver: sqlsrv.
12 database changes were checked successfully.
225 database changes did not alter table structure and were skipped.

Because the last three commits add only white spaces and " DEFAULT ''" which exists in the install sql file
IMO it can be merged without additional test.


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

@alikon
Copy link
Contributor Author

alikon commented Jan 5, 2017

Because of some regex in database fix in component installer "CREATE TABLE" requires space.
It is only needed in update sql files

we need to remeber this when we will update some others on MSSQL

@laoneo
Copy link
Member

laoneo commented Jan 7, 2017

Can a maintainer set this to RTC as we have a successful test on MSSQL.

Good work guys!

@laoneo
Copy link
Member

laoneo commented Jan 7, 2017

There will be some more work then when pr #13319 got merged. But let's get this one in first.

@infograf768
Copy link
Member

We have only one test as #12660 (comment) was done before changes

@laoneo
Copy link
Member

laoneo commented Jan 7, 2017

Honestly I'm not sure if we will find two testers for this.

@waader
Copy link
Contributor

waader commented Jan 7, 2017

I will test this one later today.

@alikon
Copy link
Contributor Author

alikon commented Jan 7, 2017

thanks @waader

@waader
Copy link
Contributor

waader commented Jan 7, 2017

I have tested this item ✅ successfully on 6fad1a2

I tested the backend with content, users and contacts and it works. Frontend is currently broken which has nothing to do with this patch. With a rudimentary fix I could get it working also in frontend.


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

@csthomas
Copy link
Contributor

csthomas commented Jan 7, 2017

You can try to test front end with applied #13262

@waader
Copy link
Contributor

waader commented Jan 7, 2017

I also did that, but it didn´t work.

@laoneo
Copy link
Member

laoneo commented Jan 7, 2017

Thanks for all the work guys!! Ready for RTC.

@jeckodevelopment
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 7, 2017
@Bakual Bakual merged commit 5d08c45 into joomla:staging Jan 7, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 7, 2017
@alikon alikon deleted the patch-85 branch January 8, 2017 06:21
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.