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

Added condition to check that we have an active connection #474

Open
wants to merge 3 commits into
base: 3.3.x
Choose a base branch
from

Conversation

oleg-andreyev
Copy link

Fixes #473

Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

Can you add a test that reproduces the problem you'd like to fix?

Collector/MigrationsCollector.php Outdated Show resolved Hide resolved
}

public function collect(Request $request, Response $response, \Throwable $exception = null)
public function collect(Request $request, Response $response, ?Throwable $exception = null)
Copy link
Author

Choose a reason for hiding this comment

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

Please note that running phpcbf is adding return type.

@oleg-andreyev
Copy link
Author

Can you add a test that reproduces the problem you'd like to fix?

It's hard to reproduce this scenario in CI, as we need to boot entire application (without configured dbal connection) and I haven't found any "functional tests" for this bundle.

@oleg-andreyev
Copy link
Author

@derrabus any chance that you'll be able to take a look at this?

@nedelyux
Copy link

@derrabus Whats problem?

@derrabus derrabus changed the base branch from 3.2.x to 3.3.x November 13, 2023 19:47
@derrabus
Copy link
Member

I'm sorry that nobody has picked up this PR after my initial triage. I still don't fully understand why problem is solved here. The collector needs a valid database connection. And without one, this whole bundle doesn't make much sense.

@oleg-andreyev
Copy link
Author

I'm sorry that nobody has picked up this PR after my initial triage. I still don't fully understand why problem is solved here. The collector needs a valid database connection. And without one, this whole bundle doesn't make much sense.

It's been awhile, let me remember.

Collector still requires an configure connection.
When starting new project and database is not yet configured, MigrationsCollector is trying to connect to database and because of PDO timeout, page feels are hanged.

IIRC initial problem is/was with clean project, when database is not yet configured.
So when I install this bundle it's automatically, it's automatically starts collecting information about executed migrations
image
and since database is not yet configured it's trying to connect to a database (hosts does not exist) and it's taking 30s (default timeout) before it's throwing an exception.

and it's not very obvious that something is wrong and developers start digging into a problem why clean project is taking 30s to load.

At least this is what I remember.

@oleg-andreyev
Copy link
Author

^^^ and the idea was: avoid collecting migrations if connection is not configured.

@oleg-andreyev
Copy link
Author

and PR that was trying to fix similar issue: #428 just wrapped code with try/catch which does not prevent of trying to make a connection

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.

MigrationsCollector still requires a active connection
3 participants