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

Don't merge config if the config has been published #751

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

keithbrink
Copy link

🔍 Changes Made

I added a condition to not run the mergeConfigFrom in the CycleServiceProvider if the config already has been published.

🤔 Rationale

If you do not have pdo_mysql installed (and likely the case with other pdo extensions), you will get an Undefined constant PDO::MYSQL_ATTR_INIT_COMMAND error, because the MysqlConfig class will get instantiated even if you remove it from the published config or set it to an empty value.

📋 Checklist

  • My code follows the contributing guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation (if necessary)
  • Tests via make test passes locally with my changes
  • [] Linting via make lint passes locally with my changes - Invalid configuration errors

🧪 Added or updated tests?

  • Yes
  • No, and this is why: Tested locally, change doesn't need a unit test
  • I need help with writing tests

@keithbrink keithbrink requested a review from lotyp as a code owner August 20, 2024 12:53
Copy link

vercel bot commented Aug 20, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
laravel-cycle-orm-adapter ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 20, 2024 0:55am

@keithbrink
Copy link
Author

@lotyp The failing tests don't seem like something that would have been affected by this change, do they?

@lotyp
Copy link
Member

lotyp commented Aug 22, 2024

@lotyp The failing tests don't seem like something that would have been affected by this change, do they?

First of all, I want to say thanks for your contribution to this project. 🔥

Regarding tests I will have a look as soon I can. Could be issue with permissions

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.

2 participants