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

BUG Default Sort table not always joined #1175

Closed
wants to merge 1 commit into from
Closed

BUG Default Sort table not always joined #1175

wants to merge 1 commit into from

Conversation

dhensby
Copy link
Contributor

@dhensby dhensby commented Feb 11, 2013

I encountered a bug when doing:

CustomPage::get()->map()->toArray();

where CustomPage has public static $default_sort = "Date"; with a column called Date in that class's $db static.

An SQL error was thrown unkown column 'Date'. This was because the table for CustomPage was not included in the query as a join or from.

This is a rather inelegant fix and I'd love some guidence on how to select the join field more appropriately, as sometimes it should be ID and sometimes RecordID.

This seems to fix the issue for me, but I'm sure it's not very robust (with things like versioning and so on)

I encountered a bug when doing:

CustomPage::get()->map()->toArray();

where CustomPage has public static $default_sort = "Date"; with a column called Date in that class's $db static.

An SQL error was thrown unkown column 'Date'. This was because the table for CustomPage was not included in the query as a join or from.

This is a rather inelegant fix and I'd love some guidence on how to select the join field more appropriately, as sometimes it should be ID and sometimes RecordID.

This seems to fix the issue for me, but I'm sure it's not very robust (with things like versioning and so on)
@dhensby
Copy link
Contributor Author

dhensby commented Feb 11, 2013

I've only tested this on 3.1, but may well be a bug in 3.0 too.

@dhensby
Copy link
Contributor Author

dhensby commented Feb 11, 2013

So, yer, this does break versioning / sitetree parts. So advice on how about to be more robust, would be good!

Despite what travis says this IS NOT GOOD TO MERGE!

Please see this gist for the error and how to replicated: https://gist.github.com/dhensby/4754499/ Deleted due to proper test in issue #2306

@dhensby
Copy link
Contributor Author

dhensby commented Feb 18, 2013

Closing as this isn't helpful to anyone yet!

@NicoHaase
Copy link
Contributor

@dhensby -- as you mention that it could break some parts: can you add additional test cases for this bug?

@dhensby
Copy link
Contributor Author

dhensby commented Apr 24, 2013

Hi @NicoHaase,

This code is not good to be merged and is just a semi "proof of concept" that it can be fixed. Tests cases on this pull request would be rather useless.

It would be possible to write a test case to prove that default_sort (or any sort) on an ancestor table's field breaks, but at the moment I don't have the time. It's easily replicated and the core team are on it.

@NicoHaase
Copy link
Contributor

Of course the testcase should cover the bug and not your fix ;)

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.

2 participants