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

Ignore legacy mapper exceptions when there is no database #79

Merged
merged 1 commit into from
Feb 10, 2017

Conversation

emodric
Copy link
Collaborator

@emodric emodric commented Feb 2, 2017

The exception thrown here breaks composer install when there is no database, for example, when doing an initial installation of a repo which includes the legacy bridge.

@emodric emodric changed the title Fix legacy mapper exceptions when there is no database Ignore legacy mapper exceptions when there is no database Feb 2, 2017
@andrerom
Copy link
Contributor

andrerom commented Feb 2, 2017

No candidates for more specifc catch?

@emodric
Copy link
Collaborator Author

emodric commented Feb 2, 2017

No candidates for more specifc catch?

Other than gracefully handling it in the kernel itself, no, not really.

Copy link
Contributor

@andrerom andrerom left a comment

Choose a reason for hiding this comment

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

Ok then :)

@emodric
Copy link
Collaborator Author

emodric commented Feb 10, 2017

Free to merge? :)

@andrerom
Copy link
Contributor

last review needed, ping @glye @bdunogier

Copy link
Member

@glye glye left a comment

Choose a reason for hiding this comment

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

"Most probable cause for error" looks dodgy, but if you have looked into it and see no more specific exception to use and no other likely error that can end up ignored by the same code, then I guess it's ok.

Wishlist: Logging and/or console warning.

@emodric
Copy link
Collaborator Author

emodric commented Feb 10, 2017

Yeah, the only time I ran into the issue was when database was not available and it only happened on first composer install of a project that has legacy bridge preinstalled.

What happens is a straight SQL execution exception, so it is not handled by eZ kernel in any way.

I can add logging though :)

@andrerom
Copy link
Contributor

andrerom commented Feb 10, 2017

So then this change could have catched that exception instead, or? (that is what both @glye and me are saying we would prefer here)

@glye
Copy link
Member

glye commented Feb 10, 2017

There's an example in the comments here for catching mysqli_sql_exception for example: http://php.net/manual/en/class.mysqli-sql-exception.php
However then you'd need to take other drivers into account, too.

@emodric
Copy link
Collaborator Author

emodric commented Feb 10, 2017

I know what you mean @andrerom and @glye, I'm not a fan of use of global exceptions, but as I said, since this happens in a very specific situation, where there is no database present and nowhere else, there is not much left to do.

One more specific error to catch would be RuntimeException (since that's the exception that ExceptionConversion gateways throw), but it's basically the same as using Exception.

@glye
Copy link
Member

glye commented Feb 10, 2017

Right. ExceptionConversion does set the title Database error, which you could check for, and re-throw the exception if it isn't set, but that's possibly even uglier ;)

@emodric
Copy link
Collaborator Author

emodric commented Feb 10, 2017

which you could check for, and re-throw the exception if it isn't set, but that's possibly even uglier ;)

It is :D ☠️

@andrerom
Copy link
Contributor

true, exception conversion :|

ok, then merge it is then.

@andrerom andrerom merged commit 7f6b0aa into master Feb 10, 2017
@andrerom andrerom deleted the legacy_mapper_exception branch February 10, 2017 11:25
@emodric
Copy link
Collaborator Author

emodric commented Feb 10, 2017

@andrerom Thanks :)

Now that this is out of the way, can I tag 1.1.1 ?

@andrerom
Copy link
Contributor

Feel free :)

@emodric
Copy link
Collaborator Author

emodric commented Feb 10, 2017

👍

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

Successfully merging this pull request may close these issues.

3 participants