xmartinj commented 10 years ago

Our SQL Server ERP software was installed with a table prefix that includes a comma. CI's Active Record automatically splits the FROM clause based on commas assuming they are separate tables ignoring quotes/brackets to keep the table name together.

i.e. "[Allen,LLC$Customer]" gets changed to "[Allen, LLC$Customer]" which fails.

p.s. The poor choice of table prefix is something we would like to change at the next opportunity, but yet this seems something the FROM function should handle better.

preems commented 9 years ago

are we planning to fix this ? I can look into this.

tlagard commented 9 years ago

Poor design choice by the original designers. Tables names should not contain commas.

mwhitneysdsu commented 9 years ago

Nearly all SQL implementations are extremely permissive when using delimited/quoted identifiers for table and column names.

Implicit join syntax (using commas in the from clause and implying the join type from the where clause) has been deprecated in the SQL standard for 22 years.

In other words, they're both terrible practices, and they both result in implementation-specific results.

What CI chooses to permit within the database libraries determines the usefulness of those libraries to developers, though. We can't always control the content and structure of the databases our sites have to use.

timw4mail commented 9 years ago

Here's my issue with trying to 'support' this:

This is a bizarre edge case that is too uncommon to 'fix', and will only inevitably lead to more bugs.

xmartinj commented 9 years ago

FWIW - we're moving away from CodeIgniter.

The primary reason for fixing this case in my opinion, is that adding spaces after commas is not a requirement for any SQL parser, and inherently assumes that a programmer didn't mean what he or she wrote.

That's just goofy.

But I understand if you want to move on to other things.

dehisok commented 9 years ago

Hello everyone!

It's my first comment on GitHub, so I'm a bit nervous :)

First of all we really need to understand how much users need this feature. As for me, it's not very big percent. But I have startup background, so I think, we need to hear every suggestion.

Here is some hardcoded variation. We can add small feature to DB config, named smth like "reserved_names".

It will look like this: $db['default']['reserved_names'][] = "[Allen, LLC$Customer]"; $db['default']['reserved_names'][] = "Really $trange TBL_NAME"; $db['default']['reserved_names'][] = "etc";

Or we can use this idea like table "aliases": $db['default']['aliases']['alc'] = "[Allen, LLC$Customer]"; $db['default']['aliases']['rstn'] = "Really $trange TBL_NAME"; $db['default']['aliases']['e'] = "etc";

And then during query building (from, join, query and maybe some another methods) we could check if there are reserved_name (or alias) for entered part of string.

So we have two ways:

Just a fix for topicstarter and users in the same situation
Idea, that can give us an opportunity to write PHP-level aliases for table names.

What do you say?

OFFTOPIC: I want to start contribute something to CI, but misunderstand how to start. As far as I understand, I just need to code smth and then make a pull request... Okay. But where is a to-do list? :) I will be grateful for small consultation for newbie :)

Thanks for your attention. I hope I will be useful for community and nearest release.

mwhitneysdsu commented 9 years ago

I think there are two parts to the problem:

Exploding the input on commas
Imploding the output with a space following the comma ', '

Removing the second part of the problem by imploding with just a comma ',' would hide most of the problems caused by the first part of the problem (but would not necessarily fix the issue). #634 is caused by the same behavior in the select() method.

The only real benefit to imploding the output with the spaces is for readability, but this is really only the case for items which are actually separate, which can't be determined by commas alone. In situations where readability is really a concern (i.e. when you're not sending the SQL to the database server), it's trivial to explode/implode the statement on commas for the purpose of inserting spaces after them, though it would make the output less useful for debugging.

Delimited_identifiers

Oddly enough, the use of commas to imply joins in the FROM clause is not only deprecated behavior in the SQL standard, it's undocumented behavior in the from() method.

@dehisok you could probably consider the issues list a to-do list of sorts, or you could ask/look on the forums.

RaymondCrandall commented 9 years ago

Hello humans. Would the rational solution be to create a feature branch for supporting bad SQL naming conventions?

mwhitneysdsu commented 9 years ago

That would probably work as a rational solution, if someone could figure out a decent implementation.

I think that some documentation/discussion of the intended handling of commas in the from() method may also be useful. Essentially, as far as I can tell, the current handling is dependent on the SQL implementation's handling of a non-standard feature. In other words, you input a comma-delimited list of tables and that's, more or less, what you get in the output. If some future version of your chosen SQL platform decides the feature has been deprecated long enough, it fails.

At that point, you either rewrite your code to build the joins properly, or you rewrite the from() method to do it for you. Maybe I'm unusual in this sense, but I prefer to not use deprecated code/behavior or, in the worst case, document it. Such documentation is especially useful in cases like this, where the end-user can make an informed choice and write more robust code, if they choose to do so.

pfote commented 9 years ago

@mwhitneysdsu which standard deprecates the use of comma? I'm aware the JOIN syntax is recommended but haven't seen any advice to actively avoid commas because the server might not have implemented it

xmartinj commented 9 years ago

If you wanted to maintain the existing behavior with from/join, there should be a fromRaw/joinRaw which would not touch the contents.

tlagard commented 9 years ago

SQL Server http://msdn.microsoft.com/en-us/library/ms175874.aspx and MySQL http://dev.mysql.com/doc/refman/5.0/en/identifiers.html

Not part of naming syntax. If they aren't saying commas are fine, then neither should CI. If you are having issues, then extend the driver accordingly. Or better yet, post this enhancement to the forums (http://forum.codeigniter.com) and let the community decide.

ivantcholakov commented 9 years ago

" There are two classes of identifiers:

Regular identifiers

Comply with the rules for the format of identifiers. Regular identifiers are not delimited when they are used in Transact-SQL statements.

Delimited identifiers

Are enclosed in double quotation marks (") or brackets ([ ]). Identifiers that comply with the rules for the format of identifiers might not be delimited. For example: ...

Identifiers that do not comply with all the rules for identifiers must be delimited in a Transact-SQL statement. For example: ...

Both regular and delimited identifiers must contain from 1 through 128 characters. For local temporary tables, the identifier can have a maximum of 116 characters. "

Comma is not explicitly mentioned, but it is a part of so-called "delimited" identifiers. It is part of the syntax.

mwhitneysdsu commented 9 years ago

From the linked SQL Server page:

 Delimited identifiers

    Are enclosed in double quotation marks (") or brackets ([ ]). Identifiers that comply with the rules for the format of identifiers might not be delimited. For example:

    SELECT *
    FROM [TableX]         --Delimiter is optional.
    WHERE [KeyCol] = 124  --Delimiter is optional.

    Identifiers that do not comply with all the rules for identifiers must be delimited in a Transact-SQL statement.

Once you have quotation marks or brackets around the identifiers, most of the rules are out the window.

The MySQL page says almost the same thing, but the delimiters are the backtick or quotation marks (if ANSI_QUOTES is enabled).

Almost every SQL platform has support for delimited identifiers, but the supported delimiters vary slightly by platform and configuration (quotation marks are the most common).

@pfote I'm still looking for the reference, but I may have confused myself with MS SQL Server's deprecation of indicating join types in the where clause with =, *=, or =*.

steveheinsch commented 9 years ago

Have you tried: $this->db->_protect_identifiers = FALSE;

//your query...

xmgcoyi commented 9 years ago

MySQL Database and table names cannot contain “/”, “\”, “.”, or characters that are not permitted in file names. http://dev.mysql.com/doc/refman/5.0/en/identifiers.html

ivantcholakov commented 9 years ago

@xmartinj The closed issue #3764 gives an idea for a workaround. If you pass the selected fields as an array what would happen now?

alex7071 commented 7 years ago

If mysql accepts dots in schema names and fixes bugs related to it, why can't you? https://bugs.mysql.com/bug.php?id=61078 This IS a bug, as you're enforcing something that is not enforced by MySql. Dot names in schema name works fine everywhere else. This is why using third party software sucks.

narfbg commented 7 years ago

If MySQL can do it, why can't we? Really?

MySQL is a huge project with countless contributors, a powerful corporation behind it, and on top of that only has to deal with itself.
CodeIgniter is a 3MB framework that sparingly gets small contributions from outsiders, is largely maintained by me alone, in my spare time, and it touches on pretty much all elements of the web technology stack.

Even isolating the issue to databases alone - we try to maintain both compatibility and ease of use between more than 10 database platforms. And the issue isn't even closed yet.

That's why.

xmartinj commented 7 years ago

No worries. We've moved to Laravel, and are moving away from this database as well.

On Mon, Mar 20, 2017, 7:01 AM Andrey Andreev notifications@github.com wrote:

If MySQL can do it, why can't we? Really?

MySQL is a huge project with countless contributors, a powerful corporation behind it, and on top of that only has to deal with itself. CodeIgniter is a 3MB framework that sparingly gets small contributions from outsiders, is largely maintained by me alone, in my spare time, and it touches on pretty much all elements of the web technology stack.

Even isolating the issue to databases alone - we try to maintain both compatibility and ease of use between more than 10 database platforms. And the issue isn't even closed yet.

That's why.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bcit-ci/CodeIgniter/issues/2755#issuecomment-287729047, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjWvVuGnolGkxBNmaC6L-zkda7jeregks5rnlybgaJpZM4BS1ur .

alex7071 commented 7 years ago

After 4 years since this has been reported without any attention, anyone can presume this "feature" will never be implemented, even though, it is a bug. Your "holier-than-thou" attitude doesn't do much to convince me you listen to your user-base, so you can abstain from snarky comments. I'm sure you're full of them.

If MySQL can do it, why can't we? Really?

MySQL is a huge project with countless contributors, a powerful corporation behind it, and on top of that only has to deal with itself. CodeIgniter is a 3MB framework that sparingly gets small contributions from outsiders, is largely maintained by me alone, in my spare time, and it touches on pretty much all elements of the web technology stack.

Even isolating the issue to databases alone - we try to maintain both compatibility and ease of use between more than 10 database platforms. And the issue isn't even closed yet.

That's why.

xmartinj commented 7 years ago

Andrey, are you maintaining primarily for your existing sites that use CI? What public sites do you have that leverage CI?

On Mon, Mar 20, 2017 at 10:05 AM alex7071 notifications@github.com wrote:

After 4 years since this has been reported without any attention, anyone can presume this "feature" will never be implemented, even though, it is a bug. Your "holier-than-thou" attitude doesn't do much to convince me you listen to your user-base, so you can abstain from snarky comments. I'm sure you're full of them.

If MySQL can do it, why can't we? Really?

MySQL is a huge project with countless contributors, a powerful corporation behind it, and on top of that only has to deal with itself. CodeIgniter is a 3MB framework that sparingly gets small contributions from outsiders, is largely maintained by me alone, in my spare time, and it touches on pretty much all elements of the web technology stack.

Even isolating the issue to databases alone - we try to maintain both compatibility and ease of use between more than 10 database platforms. And the issue isn't even closed yet.

That's why.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bcit-ci/CodeIgniter/issues/2755#issuecomment-287768553, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjWvU_n5OIhYOK1KHjbfeeA26zSxXzAks5rnoe0gaJpZM4BS1ur .

narfbg commented 7 years ago

@alex7071 I suggest you read, then read again your own reply, and repeat that excercise infinitely until you realize that my "holier-than-thou" attitude is a direct response to yours.

It's fine if you're unhappy with something, and I don't pretend that CI is perfect or even tries to satisfy everybody's wishes. Just don't act like an entitled brat and then talk about my attitude.

@xmartinj Not that I see how that is related to the issue, but no - I'm not maintaining it primarily for my own projects.

As for your second question - with all due respect, none of your business.

alex7071 commented 7 years ago

@narfbg uh-huh, i'll get on it and get back to you. Douche.

gxgpet commented 7 years ago

@alex7071 This is a public space, maybe you should never insult others...?

omphemetse commented 7 years ago

Why would one decide to include comma's or semicolon's in a table / database name? Isn't this bad practice?

alex7071 commented 7 years ago

@omphemetse How about dots? if i have the databases set up from ages ago with GB of data, like database.prod, database.dev when codeigniter had 0 problems with dots in names and i want to upgrade to a new version i have two options: NOT upgrade, or waste hours and hours of importing data in new databases, changing configs, re-testing the entire application, so i make sure some other dude did not do stupid things like hardcoding names, because of a bad decision that won't be corrected because some dude declares "it's not a bug" or "no time to fix, busy, lulz". I'm not wasting my time fixing it either because it won't ever be merged. Move on to better things seems to be the only sane choice and let this framework die in peace.

narfbg commented 7 years ago

This issue remains open only as a reminder for us in case some day we want to, and have the time and resources to implement it.

But that being said, everything that could be said here has already been said. I'm locking the conversation. And since that Alex guy is clearly only interested in ranting, he's now one of the few members of our block list.

narfbg commented 2 years ago

Ok, let's officially end this ...

For the record, I don't like not having this feature. But attempting to implement it comes with too many potential problems to solve and it's just not happening. All tools come with some limitations and its time we all accepted this limitation.

bcit-ci / CodeIgniter

Table names with commas. #2755