Llewellynvdm
commented
1 year ago Thank you for your response, @kamil-tekiela. We apologize for the delay in our reply; we are currently short on volunteers and are doing our best to keep up with all the requests.
We appreciate your effort in improving the code, and we've reviewed your changes. While most of them seem straightforward, we think that the pull request contains too many changes at once. It would be helpful if we could isolate the changes into smaller pull requests, each targeting a specific issue or feature. This would make it easier to review and test the changes and minimize the risk of unintended side effects.
We understand that breaking down the PR into smaller ones may require more work on your part, but we believe it's a worthwhile effort to ensure the quality and maintainability of the code. We suggest starting by identifying the different changes that were made in the original PR and grouping them into separate PRs based on their nature or priority.
For example, here are some possible grouping options:
- Spelling and grammar fixes
- Typehinting updates
- Refactoring and simplification of code
- Introduction of null coalescing operator
- Changes related to error handling or validation
- API improvements or additions
Of course, the actual grouping may depend on the specifics of the code and the needs of the project.
Again, thank you for your contribution, and we look forward to working with you to improve the code.
This is an attempt to modernize the mysqli driver in Joomla. I am not sure if I am doing this the right way. I was not able to execute PHPCS and I see absolutely zero static analysis setup in this project. I run Psalm with level 4 and it returned a bunch of errors, which I fixed in the mysqli directory, but I didn't touch the rest.
This PR contains a few fixes I found through static analysis and manual reading through the code. It also contains simplifications to the existing code, which still contained workarounds for PHP < 5.6 despite composer claiming that the minimum version is PHP 7.2. I switched over to exception mode as I noticed that the PDO driver uses exceptions, so I see no reason why mysqli shouldn't. I removed reference binding, which I believe isn't needed anymore, but I wasn't able to test it properly as I have absolute no idea how to bind parameters in this project. There's a
setQuery()method andexecute()but there's no method to bind? The tests are also very limited and I doubt they test edge cases or even the happy path. I got no errors when I run PHPUnit on my machine.This is my first contact with anything Joomla related. I installed Joomla today, but I wasn't able to yet figure out how this DBAL is utilized by Joomla or by its plugins. I appreciate feedback on this PR. I did a similar review in Doctrine DBAL some time ago, and I wonder why Joomla isn't using Doctrine DBAL.