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

Throw exceptions on SQL errors #18151

Merged

Conversation

cedric-anne
Copy link
Member

@cedric-anne cedric-anne commented Oct 29, 2024

Checklist before requesting a review

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my code.
  • I have added tests that prove my fix is effective or that my feature works.
  • This change requires a documentation update.

Description

Since we did not have a centralized error catching in the past, we were not able to throw exceptions when a SQL error occurred. They were just logged and were not stopping the code execution, and this could have the effect of triggering hard-to-debug errors in subsequently executed code.

Now we have a centralized error catching system, we can throw exceptions when a SQL error is triggered. It would result in displaying the error page. For instance, if I force a SQL querry error in the impact analysis tab:
image

@cedric-anne cedric-anne self-assigned this Oct 29, 2024
@cedric-anne cedric-anne force-pushed the 11.0/exception-on-sql-errors branch from 1c82c4a to f79da9f Compare October 31, 2024 09:58
@cedric-anne cedric-anne added this to the 11.0.0 milestone Oct 31, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
@cedric-anne
Copy link
Member Author

E2E tests are failing due to the SQL error reported in #18175.

Copy link
Contributor

@AdrienClairembault AdrienClairembault left a comment

Choose a reason for hiding this comment

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

With these changes, should Toolbox::logSqlError() and the associated sql-errors.log file be depreciated/removed ?

With the new exception, SQL errors end up directly in the php-errors-log file.
I don't see a use case for manually calling Toolbox::logSqlError().

@cedric-anne
Copy link
Member Author

With these changes, should Toolbox::logSqlError() and the associated sql-errors.log file be depreciated/removed ?

With the new exception, SQL errors end up directly in the php-errors-log file. I don't see a use case for manually calling Toolbox::logSqlError().

sql-errors.log still contains SQL warnings and SQL debug info. We could change it, but it is out of the scope of the current PR.

@trasher
Copy link
Contributor

trasher commented Nov 4, 2024

Any reason this one is still draft, or should me merge (e2e error is not a blocker)?

@cedric-anne
Copy link
Member Author

Any reason this one is still draft, or should me merge (e2e error is not a blocker)?

The e2e tests fails due to an SQL error that is now blocking, see #18175. We have to fix this SQL error before being able to merge the current PR.

@cedric-anne cedric-anne force-pushed the 11.0/exception-on-sql-errors branch from 08d23ff to d7de9a6 Compare November 4, 2024 10:52
@cedric-anne cedric-anne force-pushed the 11.0/exception-on-sql-errors branch from d7de9a6 to 24d2593 Compare November 4, 2024 10:58
@cedric-anne cedric-anne force-pushed the 11.0/exception-on-sql-errors branch from 24d2593 to be96238 Compare November 4, 2024 12:44
@cedric-anne cedric-anne marked this pull request as ready for review November 4, 2024 14:41
@cedric-anne
Copy link
Member Author

I added a commit to quickly adapt the install/update process.

src/Update.php Show resolved Hide resolved
@cedric-anne cedric-anne merged commit 9033804 into glpi-project:main Nov 5, 2024
7 checks passed
@cedric-anne cedric-anne deleted the 11.0/exception-on-sql-errors branch November 5, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants