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

[9.x] Add getAllTables support for SQLite and SQLServer schema builders #41896

Merged
merged 2 commits into from
Apr 12, 2022
Merged

[9.x] Add getAllTables support for SQLite and SQLServer schema builders #41896

merged 2 commits into from
Apr 12, 2022

Conversation

jhavenz
Copy link
Contributor

@jhavenz jhavenz commented Apr 9, 2022

This PR adds support for calling \Illuminate\Support\Facades\Schema::getAllTables() on SQLite and SQLServer connections.

I'm returning the name and type columns to maintain consistency with what's already being returned when using the MySQL or Postgres connections.

P.S.
This being my 1st Laravel contribution, please go easy on me if I've done something incorrectly here. 😉

@GrahamCampbell GrahamCampbell changed the title add getAllTables support for SQLite and SQLServer schema builders [9.x] Add getAllTables support for SQLite and SQLServer schema builders Apr 9, 2022
@jhavenz
Copy link
Contributor Author

jhavenz commented Apr 9, 2022

eof lines have been fixed. Apologies.

@taylorotwell taylorotwell merged commit 827893f into laravel:9.x Apr 12, 2022
@taylorotwell
Copy link
Member

@Sourcefli

Please fix this failing test and explain why it is failing: https://github.com/laravel/framework/runs/6003485596?check_suite_focus=true

Will give it about 24 - 48 hours to be fixed before I revert this PR. Thanks.

@jhavenz
Copy link
Contributor Author

jhavenz commented Apr 13, 2022

No problem, thanks @taylorotwell.

Strange that in the test theres a count of 7 though, being that only up to 3 tables - each with no more than 3 columns in their schema - is being used within each test... and these tests were passing when run by themselves or within their respective file's tests. My suspicion is that there's leftover tables from previous tests that ran.
Will fix this shortly.

@driesvints
Copy link
Member

We need to revert this asap if we can't find a solution now. Every single PR to 9.x is currently failing because of this.

@jhavenz
Copy link
Contributor Author

jhavenz commented Apr 14, 2022

I found that the DB_CONNECTION is still testing when running the tests using this command:
vendor/bin/phpunit tests/Integration/Database --verbose --exclude-group SkipMSSQL

I was using the phpunit.xml.dist file to run these tests locally, yet came to realize that this file was ignored when running the tests using the command above.

I have updated the DatabaseSqlServerSchemaBuilderTest::getEnvironmentSetUp() to use the sqlsrv connection.

@driesvints
Copy link
Member

@Sourcefli can you PR?

@jhavenz
Copy link
Contributor Author

jhavenz commented Apr 15, 2022

Ok, I've got up the fix to this PR, #41984.

I was keep pushing up changes because I had had passing tests locally, so I could only see if/when it failed after I'd made some edits then pushed again.

Curious to know how you guys typically go about setting up multiple db connection envs when working with this test suite, which would 1) stay in sync with the envs used while testing different connections on Github, and 2) be ignored when pushing. I ended up jumping into the orchestra vendor folder for this reason, though not sure if that's the ideal solution.

Thanks for your assistance guys 👍

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.

None yet

5 participants