Refactoring #73
Refactoring #73
Conversation
…mit fix this and still prevents the warnings.
The goal is to completely deprecate the helper plugin in favor of instantiating SQLiteDB
Query saving has been moved to its own class
|
@annda FYI you might want to look at this when debugging the struct migration stuff. |
… the SQL query is executed
Multiple statements are only executed with PDO::exec() not with a prepared statement
This ensures that the helper is working as intended for backwards compatibility. New tests for the new class still have to be added.
PLUGIN_SQLITE_QUERY_EXECUTE
Feature event query execute
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This is supported natively in sqlite now
|
This should now be ready to merge. @annda I would like to run this branch on a bunch of our wikis first, to see if there are any issues with the backwards compatibility. Once we're satisfied that it works, we can merge and start migrating our plugins to use the new class. |
|
Could you do please also a update of the documentation if it merged, with some guiding what is updated? |
|
@Klap-in absolutely! docs will be updated as soon as this is merged. |
|
The new code wraps each migration (regardless if it's a normal file based one or an event based one) into a transaction and properly rolls it back if an exception happens. This however means that migrations may not have any transactions themselves because nested transactions are not supported by sqlite. Currently I am aware of the following plugins doing their own transactions in migrations:
The current docs do not mention to use transactions in migrations and it seems no other plugins used them. So I would suggest to adjust the plugins above instead of changing the behaviour of the SQLiteDB class. |
|
This PR dropped the custom implementation of We rely on Some other plugins also use the function and may have to be adjusted. |
Even though mutliple statements can be passed to $sth->execute(), failures will only be checked in the very first statement. Failures later on will lead to the rest of the statements to be silently ignored rendering our rollback strategy moot. We now always split multiline statements and run them separately (affects import and migrations). The export was adjusted to not include the transaction handling and fix entry exports.
PDO::prepare sometimes throws ValueErrors (and maybe other Throwables) so we convert them to PDOExceptions
This is clearer in what to expect as a return value
|
The correct syntax to use when you don't want duplicates would be
Workarounds on this limitation directly in the SQL are outlined above (sub selects) but I guess fixing it in code after retrieving the results is often easier. I don't think it's worth to fix backwards compatibility for this and would rather have plugin authors adjust their code should it be affected. |
This is a common use case in plugins
Since using GROUP_CONCAT with a custom separator and the DISTINCT keyword is still broken in sqlite (and might never be fixed), this readds our custom implementation back. Using a different name than the default allows developers to pick the native implementation when adequate. See also #73 (comment) This reverts commit ff1cb7a.
This is what the old query methods allowed. Passing in an array is still somewhat cleaner, but allowing separate arguments should make porting older code a bit more convenient.
|
Hy Andi, I just made the migration. I had no time to check the new beauty structure. During the update, just for your information, I got it by the cursor cache data. Cheers |
Deprecate the old helper class in favor of a new
dokuwiki\plugin\sqlite\SQLiteDBwith a cleaner API and proper use of exceptions. Removal of all sqlite2 support, only PDO::sqlite is supported. Major code cleanup. Merge and cleanup of #65