Skip to content

Fixed regular expression to get password from dsn #12448

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

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

SakiTakamachi
Copy link
Member

@SakiTakamachi SakiTakamachi commented Oct 16, 2023

cc: @devnexen
The regular expression was wrong

take2 of: #12446

@devnexen
Copy link
Member

ah that happens we ll get there :-) I ll review in couple of hours cheers.

@SakiTakamachi
Copy link
Member Author

I don't want to merge to master too many times and fail, so I'm trying to copy the branch to my CI and run it.

Can you see this?

https://app.circleci.com/pipelines/github/SakiTakamachi/php-src/2/workflows/fdb0b962-9c7c-4657-a81f-0157f93b6fa2/jobs/2

@SakiTakamachi
Copy link
Member Author

My CI passed

@devnexen
Copy link
Member

Thanks again !

@devnexen devnexen merged commit 47c6b3b into php:master Oct 16, 2023
@mbeccati
Copy link
Contributor

mbeccati commented Oct 17, 2023

My CI is not passing as I have no password (using peer authentication for tests). I will look into a fix, or adding a skipif.

For reference: https://revive.beccati.com/bamboo/browse/PHP-SRC-TESTS-3559/test/case/76220624

@SakiTakamachi
Copy link
Member Author

Ah, I overlooked that case. If the password cannot be retrieved from the DSN, it should have passed null instead of skipping.

@SakiTakamachi
Copy link
Member Author

No, it seems better to skip when using peer connect. This test looks at the behavior when there is not peer connection.

@mbeccati
Copy link
Contributor

Yeah, I guess you're right. It's not possible to identify what kind of authentication is going to be used, so I'd suggest skipping if there's no password in the DSN.

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