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

[Feature] Add SQL Data Source #379

Merged
merged 8 commits into from
Sep 9, 2024
Merged

[Feature] Add SQL Data Source #379

merged 8 commits into from
Sep 9, 2024

Conversation

msoroka
Copy link
Contributor

@msoroka msoroka commented Nov 16, 2023

Originally created by LemonMind and TorqIT on Data Importer Extension.

All explained in attached docs.

@mattamon
Copy link
Contributor

Hey @msoroka

Unfortunately the static analysis failed, could you please recheck your code and fix the tests.

@mattamon mattamon self-assigned this Aug 26, 2024
@mattamon
Copy link
Contributor

mattamon commented Sep 4, 2024

Hey @msoroka are you still working on this? It seems like a pretty cool feature, but with failing tests we cannot merge it.

Could you give me a quick comment, that you are going to fix this? Otherwise I will close the PR in a week. If you still want this, we can reopen the PR anytime then.

@msoroka
Copy link
Contributor Author

msoroka commented Sep 4, 2024

@mattamon Yes, I'll do it till end of this week.

@mattamon
Copy link
Contributor

mattamon commented Sep 4, 2024

@mattamon Yes, I'll do it till end of this week.

Awesome thank you!

@msoroka msoroka force-pushed the 1.x branch 2 times, most recently from 428443f to be5b90c Compare September 4, 2024 09:10
@msoroka
Copy link
Contributor Author

msoroka commented Sep 4, 2024

@mattamon Done

@mattamon mattamon added this to the 1.10.0 milestone Sep 9, 2024
Copy link
Contributor

@mattamon mattamon left a comment

Choose a reason for hiding this comment

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

I checked out and tested the PR now.
Thank in advance! It looks pretty solid and is a neat feature!

Works like expected at least for my tests. One minor change, could you please add the declare(strict_types=1); to every php file.

Copy link
Contributor

@mattamon mattamon left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for your patience and time!
Really appreciate this new feature!

@mattamon mattamon merged commit 2d6bd8f into pimcore:1.x Sep 9, 2024
8 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants