WIP added db insert ignore support

[tada5hi]

Each pull request should address a single issue, and have a meaningful title.

Description
Added required insert ignore support for sqlite3 and mysql.

Checklist:

• Securely signed commits
• Component(s) with PHPdocs
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[lonnieezell]

A couple of things here:

1. Would prefer new insertIgnore method added instead of just adding a new argument. The old database layer we've ported has too many arguments already. :)
2. Any new commands would have to support all database platforms we support, if possible, and throw en error about unsupported feature if it doesn't. You've missed PostgreSQL here.
3. We could like contributions to be GPG signed please. The user guide has docs for contributors.

[tada5hi]

Hi, thanks your reply.
1.
Hi, yes of course, but that will nearly occure double function code which is nearly the same.
But im fine with that.
Should i create also a method insertIgnore(), insertBatchIgnore() in the main BaseBuilder Class or just add them to the mysql/sqlite/postgresql builder class?

I saw that you abstract methods which are the same for most databases in the BaseBuilder Class. But the insertIgnore statement is a little bit different for mysql and sqlite. Should i still create these methods in the basebuilder class but for example with empty function body or should i just create the methods in the builder class of mysql,sqlite,.. ?

Should i also create new protected methods _insertIgnore(), _insertBatchIgnore() or just new methods for the main public methods?

2. Yes, PostgreSQL is not supporting insert ignore.
3. I will ;) Im currently trying to sign my already pushed commits. Till now i just was able to sign my latest commit ^^

[lonnieezell]

Perhaps a better way is to make a new ignore() method that sets a class flag, which is false by default. Then, all of those methods can check the status of that flag to determine if they should be ignored. I think that's cleaner, and definitely better than a lot of new methods, and makes it simple for non-supporting drivers to ignore.

[tada5hi]

Yes think so too ;)
If you are fine with file changes, i will update the user guide.

[lonnieezell]

Yes, I'm fine with the changes. They look good. And you reset $ignoreInsert after the inserts, too - so great!

Please update docs and add a test for it.

[tada5hi]

Yes, but first i have a little question. I found out that the ignore statement is not just valid for insert statements in case of mysql and sqlite. Here in short which driver supports the ignore statement in select, insert, ...
Select: Mysql
Insert: Mysql, Sqlite
Update: Mysql, Sqlite
Delete: Mysql
So i was thinking about to just modify the _insert(), _update(), ... methods in the BaseBuilder and to add a function called compileIgnore($statement) which Returns an empty String in the basebuilder, and in the driver Class we also create that Method and in case the driver class supports the ignore Option for that Statement it will return the ignore String for example 'IGNORE' in case of MySQL. So we keep double code as low as possible. what do you think ?

[lonnieezell]

I'm fine with the idea in general, but it has to work in a broader way, where each database can modify the query as needed. For example. INSERT IGNORE is supported in Postgres 9.5+ through ON CONFLICT DO NOTHING|UPDATE. Other databases may need similar additions when added. So, when coming up with a solution, please keep that in mind, and get it working in Postgres.

[tada5hi]

I think tests are kinda useless in that case @lonnieezell ,because the ignore statement is individual per Driver.
And the MockConnection Class which is used for tests, just create an instance of MockBuilder instead of the specific Database Builder (f.e MySQLi Builder Class). Thats why the compileIgnore() function always returns an empty string, because the supported ignore statements are declared in the Driver Class.

This is also why there is no check for the DBDriver in case of the Replace Test, because the specification for the Replace query of sqlite is made in the driver builder class. ('INSERT OR REPLACE' instead of just 'REPLACE).
So in the test environment all Drivers going to have the same query syntax.

[lonnieezell]

We have live database tests that run on Travis in each of the databases, so the tests aren't worthless, and they make sure it works with the actual database in a real-world situation.

[tada5hi]

docs are missing before a merge is possible ?

[tada5hi]

@lonnieezell What do you think ?

[MGatner]

@tada5hi I'm not great with Coveralls but it looks to me like this PR is failing because it thinks you changed a lot more than you did because of all the random spacing changes. Could you revert changes to files that weren't actual code changes (e.g. system/Common.php and system/Debug/Toolbar/Views/toolbarloader.js.php) and merge the current develop branch and see if that makes the difference?

[jim-parry]

Looks good to me, except for the _insert and _insertBatch methods in Postgre/Builder ... those don't seem related to this PR at all.

[MGatner]

I think the content of this one is good, but it has lingered and now needs conflict resolution and maybe some cleanup. @tada5hi are you still available and willing to touch it up a bit? I think we're really close.

[tada5hi]

sorry @MGatner missed that mention.

[MGatner]

No worries! Could you glance at #2446 and make sure it's still all good, in your opinion?

[tada5hi]

loooks good ;)