Fix for php 8.1 compatibility #330
Conversation
QuickBooks/Driver/Sql/Mysqli.php
Outdated
| } | ||
|
|
||
| return $this->_conn->real_escape_string($str); | ||
| return $this->_conn->real_escape_string($str ?? ''); |
There was a problem hiding this comment.
NOTE: Please do this place and similar places in this PR, where ?? is used.
Usage of the null coalescing operator (??) on the PHP < 7.0 would result in a syntax error.
Please adjust the code to also work on PHP 5.x (according to composer.json this library supports PHP 5.x).
For this particular place maybe it's better to return NULL, when $str === null, but since there are no automated tests, this might break things :(
There was a problem hiding this comment.
I used isset() ? : instead...
|
Please also remove |
Done. |
|
Looks perfect to me. @xtremevision , Have you tested these changes in a production environment (e.g. in a project that runs on PHP 8.1+, where this library is used)? |
|
Tbh I can't remember. A lot of time has passed since I placed this PR. I can't remember if I used it and/or where. I'll check. |
QuickBooks/XML.php
Outdated
| ); | ||
|
|
||
| return str_replace(array_keys($transform), array_values($transform), $str); | ||
There was a problem hiding this comment.
This is the last change. Please remove this trailing space.
|
I hope that once this PR is merged. The library would be work on PHP 8.1+. For people using this library, that should be a big relief. If possible, consider switching to the official QuickBooks API client for PHP: https://github.com/intuit/QuickBooks-V3-PHP-SDK . |
|
Done. Removed trailing space. |
You've removed the wrong trailing space. There should be one empty file at the end of each file. I was talking about the empty line above the |
|
Sorry, try now. |
|
Great. Thank you @xtremevision . |
Many people probably would if this SDK supported QB Desktop. |
|
The problem is installing it using composer from a custom GitHub repo. With the original name composer couldn't find my forked repo.
I'll check the my fork for any commits and perhaps we can merge everything and I can change it back to the original name?
…
On Jul 6, 2025 at 7:20 AM, Alex ***@***.***> wrote:
@aik099 requested changes on this pull request.
In composer.json:
> @@ -1,5 +1,5 @@ { - "name": "consolibyte/quickbooks", + "name": "xtremevision/quickbooks-php",
Please revert this line.
If you need to test how your changes work on your project, then you can always override used repository in your project's composer.json file.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Unfortunately I am using another module that relies on this repo. Switching to another sdk is sadly impossible. |
|
I would be very grateful if the PR could be merged finally so I can update my composer and reinstall the original module instead of my fork. The original PR was submitted over 1 year ago. Thanks. |
|
Hello? @aik099 is there a hold-up? I believe I've made all the requested changes. Can the merge be done please? Thanks. |
|
@xtremevision, I've approved the PR, but I don't have merge rights in this repo. @keith-chargeover, could you please merge this? |
3 participants
Fixed array notation curly brackets