Skip to content

Conversation

rkopack
Copy link
Contributor

@rkopack rkopack commented May 15, 2019

Adding in two new functions to ..\ext\sqlite3\sqlite3.c and a new PDO Attribute

lastExtendedErrorCode() - calls sqlite3_extended_errcode instead of sqlite3_errcode
toggleExtendedResultCodes(bool) - calls sqlite3_extended_result_codes

PDO_SQLITE_ATTR_EXTENDED_RESULT_CODES - new Attribute used in pdo_sqlite_set_attr in ../ext/pdo_sqlite/sqlite_driver.c

… Attribute

lastExtendedErrorCode() - calls sqlite3_extended_errcode instead of sqlite3_errcode
toggleExtendedResultCodes(bool) - calls sqlite3_extended_result_codes

PDO_SQLITE_ATTR_EXTENDED_RESULT_CODES - new Attribute used in pdo_sqlite_set_attr in ../ext/pdo_sqlite/sqlite_driver.c
@petk petk added the Feature label May 16, 2019
Copy link
Member

@KalleZ KalleZ left a comment

Choose a reason for hiding this comment

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

Also there is no tests for these new methods and attribute option, all of this needs to be covered and make sure that no other tests fail due to these additions.

If you need help writing test cases, you can read more about that on the QA website:
http://qa.php.net/write-test.php

…_INFO_EX. Correctly registering PDO class const.
@rkopack rkopack force-pushed the sqlite3-extended-error-codes branch from 4869458 to 5ecbf6c Compare May 17, 2019 18:24
@rkopack
Copy link
Contributor Author

rkopack commented May 17, 2019

@KalleZ I've added in the required things I missed (Thank you! I thought those functions looked important...) and i'm in the process of writing the UTs for these now.

@rkopack rkopack force-pushed the sqlite3-extended-error-codes branch 2 times, most recently from 014e55b to 3633c60 Compare May 17, 2019 18:49
… lastExtendedErrorCode, and pdo_sqlite PDO_SQLITE_ATTR_EXTENDED_RESULT_CODES
@rkopack rkopack force-pushed the sqlite3-extended-error-codes branch from 3633c60 to 6a548a1 Compare May 17, 2019 18:56
@rkopack
Copy link
Contributor Author

rkopack commented May 17, 2019

Hmm, the UTs are acting differently than how they do on my test machine. I think it has to do with some SQLlite3 version differences. I'll have to play around and check it out.

…ublicly for PDO_SQLITE_ATTR_EXTENDED_RESULT_CODES
@rkopack rkopack force-pushed the sqlite3-extended-error-codes branch from f0bd8ab to db046a3 Compare May 17, 2019 19:44
@KalleZ
Copy link
Member

KalleZ commented May 17, 2019

@rkopack Thank you for the changes, lets see what the CI tools say now :)

@rkopack
Copy link
Contributor Author

rkopack commented May 20, 2019

@KalleZ Looks like the UT are all good now (at least according to the CI tools) so let me know if you think they are acceptable in terms of coverage. I know there's been some discussion on the mailing list about possibly changing the SQLite3::enableExceptions() function and depending on where that goes I would change the toggleExtendedResultCodes() to follow suit (in terms of function prototype and return)

I still need to look around about the return codes with regards to UT and checking returns but I think we don't need to do anything because, by default, the extendedResultCodes are turned off. Because result codes are unique per connection (including the errmask for result codes) I would like to believe that if somebody were to turn them on somewhere in the code they would check for the extended codes from that. But I am probably not fully understanding the concern that was brought up because of my lack of familiarity with the codebase.

@rkopack rkopack closed this May 20, 2019
@rkopack rkopack reopened this May 20, 2019
@rkopack
Copy link
Contributor Author

rkopack commented May 20, 2019

Whoops that was the wrong button ... you think it'd give you an "Are you sure?"!

@KalleZ
Copy link
Member

KalleZ commented May 22, 2019

Hi! Sorry I have been busy the last few days, but thinking about it, then I think we should strive for consistency despite the original naming is not ideal imo, but it is what it is so I think we should go along the enableExtendedResultCodes() to be in line with enableExceptions() even though I like your naming better.

As for the enableExceptions() issue, I will try tackle this soonish on my own.

What are your thoughts?

@rkopack
Copy link
Contributor Author

rkopack commented May 22, 2019

Sounds good to me, Kalle! I'll make the changes for my code sometime this week.

@KalleZ
Copy link
Member

KalleZ commented May 22, 2019

Thank you! Just let me know when you want a review and ping/tag me and I will take a look

Thanks for your work

…f enableExceptions for consistency. The new function name is enableExtendedResultCodes
@rkopack rkopack force-pushed the sqlite3-extended-error-codes branch from 70082f0 to 32b3246 Compare May 22, 2019 21:54
@rkopack
Copy link
Contributor Author

rkopack commented May 23, 2019

@KalleZ I actually had some time to make the change yesterday so I believe the code is in a good place now. Let me know what your thoughts are!

And thank you for your work too! I have to say, I was initially nervous about jumping into this but it's been a very good experience so far and part of that has been how kind the tone has been from everybody I've spoken to.

@rkopack
Copy link
Contributor Author

rkopack commented May 30, 2019

Hello again it's been about a week and just wanted to give this a bump.

@nikic
Copy link
Member

nikic commented Jun 21, 2019

To clarify, is this what the PR implements?

  • A PDO::lastExtendedErrorCode() method, which is always available.
  • A SQLITE_ATTR_EXTENDED_RESULT_CODES option, which can be enabled with setAttribute() and makes the extended error code available in lastErrorCode().
  • A PDO::enableExtendedResultCodes() method that does the same as enabling the aforementioned attribute.

If so, I feel like there is quite a bit of duplication here. If we have the attribute, why do we also need the extra enable method? Do we need both the ability to make the existing error code interfaces return extended codes and have a separate method to fetch them (possibly we do, I don't know)?

@rkopack
Copy link
Contributor Author

rkopack commented Jun 21, 2019

To clarify, is this what the PR implements?

  • A PDO::lastExtendedErrorCode() method, which is always available.
  • A SQLITE_ATTR_EXTENDED_RESULT_CODES option, which can be enabled with setAttribute() and makes the extended error code available in lastErrorCode().
  • A PDO::enableExtendedResultCodes() method that does the same as enabling the aforementioned attribute.

That is slightly incorrect. The following functionality is being added:

sqlite3::lastExtendedErrorCode
sqlite3::enableExtendedResultCodes
PDO sqlite3 attribute SQLITE_ATTR_EXTENDED_RESULT_CODES

The 2 sqlite3 methods will allow users of that class to both fetch and enable/disable the extended error code functionality. The sqlite3 PDO attribute will allow users of the that class to enable/disable the extended error code functionality.

I should probably add in a way for the PDO to fetch the extended error functionality as well however I don't know the best way to expose the functionality.

@nikic
Copy link
Member

nikic commented Jun 21, 2019

Oh sorry, I totally missed that this is working on two separate extensions. This makes a lot more sense to me now...

@nikic
Copy link
Member

nikic commented Jul 2, 2019

Merged as b546ae9 into 7.4. Thanks, and sorry for the delay!

@nikic nikic closed this Jul 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants