Skip to content

[RTM] Fix issue #1376 - catch db error on bootup#1378

Merged
stefanheimes merged 1 commit intohotfix/2.1.9from
hotfix/issue-1376
Apr 2, 2020
Merged

[RTM] Fix issue #1376 - catch db error on bootup#1378
stefanheimes merged 1 commit intohotfix/2.1.9from
hotfix/issue-1376

Conversation

@discordier
Copy link
Copy Markdown
Member

Description

When there has been an error checking the tables on bootup, the
exception was not being catched. Instead it broke the whole install
process due to the newly added migrations.

Checklist

  • Read and understood the CONTRIBUTING guidelines
  • All tests passing
  • Checked the changes with phpcq and introduced no new issues

When there has been an error checking the tables on bootup, the
exception was not being catched. Instead it broke the whole install
process due to the newly added migrations.
@discordier discordier self-assigned this Mar 10, 2020
@discordier discordier added the bug A bug! A bug! Fast, squish it! label Mar 10, 2020
@discordier discordier added this to the 2.1.9 milestone Mar 10, 2020
@discordier discordier linked an issue Mar 10, 2020 that may be closed by this pull request
Copy link
Copy Markdown
Contributor

@dmolineus dmolineus left a comment

Choose a reason for hiding this comment

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

I would only silencing Doctrine\DBAL\DBALException here but otherwise LGTM

$this->logger->error('MetaModels startup interrupted. Not all MetaModels tables have been created.');
return;
}
} catch (\Throwable $throwable) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any reason silencing \Throwable and not only Doctrine\DBAL\DBALException?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No serious reason, aside from the fact that we want to interrupt booting entirely when anything here goes wrong.
We already did so in Contao 3, as there were multiple reasons where we would want to abstain.

I do not know what the upcoming changes in doctrine will bring, as they are removing the single common repo and therefore thought it could be wise to simply abstain on all errornous situations.

Copy link
Copy Markdown
Member

@richardhj richardhj left a comment

Choose a reason for hiding this comment

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

LGTM with catchin Throwable

@zonky2 zonky2 changed the title Fix issue #1376 - catch db error on bootup [RTM] Fix issue #1376 - catch db error on bootup Mar 23, 2020
@zonky2
Copy link
Copy Markdown
Contributor

zonky2 commented Mar 23, 2020

@stefanheimes pls merge and tag

@stefanheimes stefanheimes merged commit 26ecff1 into hotfix/2.1.9 Apr 2, 2020
@stefanheimes stefanheimes deleted the hotfix/issue-1376 branch April 2, 2020 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug A bug! A bug! Fast, squish it!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SQLSTATE[3D000]-error during installation MM 2.1.8 and C4.4.47

6 participants