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

Driver for SQLite3 #840

Closed
wants to merge 4 commits into from
Closed

Driver for SQLite3 #840

wants to merge 4 commits into from

Conversation

BenMorel
Copy link
Contributor

This PR adds an additional driver using the SQLite3 class, in addition to the PDO_SQLITE driver.

Why this driver?

Even though the PDO SQLite driver is already available to interact with SQLite databases, PDO currently has a big limitation: it does not allow to load SQLite extensions.

This functionality is provided by the SQLite3 class, which becomes your only option if you need to use your PHP application with an SQLite extension such as SpatiaLite.

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DBAL-1208

We use Jira to track the state of pull requests and the versions they got
included in.

@@ -49,6 +49,6 @@ public function getSQLDeclaration(array $fieldDeclaration, AbstractPlatform $pla
*/
public function convertToPHPValue($value, AbstractPlatform $platform)
{
return (null === $value) ? null : $value;
return (null === $value) ? null : (string) $value;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to modify this type because this test was still failing. This is because SQLite3 returns numeric values as float for numbers that can be represented as a floating-point value without loosing precision, and as string for others.

I'm not sure if this line should be updated or if the test should be modified instead, but I suppose this should be all right as the Decimal type is always supposed to return a string.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should fix this in the driver for now. It is a known problem that we do not like anything else then strings.
I think beberlei have some plans to fix it in the future. If we lucky someone will do it when they get annoyed enough by it :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remove this change and the test passes. Not sure why. I'm going to branch and rebase this PR with this change reverted for now and see if the tests pass on travis.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zeroedin-bill That's weird, please keep me updated!

@BenMorel
Copy link
Contributor Author

Any feedback on this PR?

/**
* @var array
*/
protected $_userDefinedFunctions = array(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no leading underscore and private visibility please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, just copy/pasted that from another driver. Fixed.

@kimhemsoe
Copy link
Member

👍 when the underscore issue have been resolved

@BenMorel
Copy link
Contributor Author

BenMorel commented Jun 7, 2015

Good for me!

@BenMorel
Copy link
Contributor Author

BenMorel commented Jul 2, 2015

Ping @kimhemsoe , can this PR be merged?

@BenMorel
Copy link
Contributor Author

Ping @stof @kimhemsoe , what's preventing this PR from being merged, 7 months later?

@BenMorel
Copy link
Contributor Author

Hi guys, I've worked hard on this PR (and others), and it's a pity no one seems to care about them.
Could you please review and at least tell me if there is a problem preventing you from merging this?

@stof
Copy link
Member

stof commented Dec 16, 2015

@BenMorel I'm not a merger on the DBAL repo

@billschaller
Copy link
Member

See #2264

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants