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

Mysqli redesign #263

Open
wants to merge 8 commits into
base: 2.0-dev
Choose a base branch
from

Conversation

kamil-tekiela
Copy link

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 and execute() 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.

@Llewellynvdm
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants