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

[BUGFIX] Add custom CockroachSchemaManager and override selectTableColumns() / fixes #35 #36

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Vision42
Copy link

@Vision42 Vision42 commented Mar 12, 2024

Changes In Code

Add custom CockroachSchemaManager extending the PostgreSQLSchemaManager and override the selectTableColumns() function to add the a.attisdropped = false flag to the SQL query.

Additionally override compileColumns function in CockroachGrammar-Class to fix the output of the Schema::getColumns function.

Issue ticket number / Business Case

#35

Checklist before requesting a review

  • I have written PHP tests.
  • I have updated the documentation in the readme where needed.
  • I have checked code styles, PHPStan etc. pass.
  • I have provided an issue/business case.

@Vision42 Vision42 requested a review from peterfox as a code owner March 12, 2024 07:38
@Vision42 Vision42 changed the title [TASK] Add failing tests for issue #35 [BUGFIX] Add custom CockroachSchemaManager and override selectTableColumns() / fixes #35 Mar 12, 2024
Add a.attisdropped = false flag to compile columns sql to fix the output of Schema::getColumns().
Add custom cockroach db driver for loading the custom cockroach schema manager to fix renaming issue ylsideas#35
@Vision42 Vision42 force-pushed the task/35-renaming-columns-with-deleted-columns branch from 7d960bf to 1d178bc Compare March 12, 2024 14:24
Copy link

codecov bot commented Mar 12, 2024

Codecov Report

Attention: Patch coverage is 92.30769% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 93.12%. Comparing base (68c2628) to head (c97190c).
Report is 1 commits behind head on main.

Files Patch % Lines
src/Schema/CockroachSchemaManager.php 86.95% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main      #36      +/-   ##
============================================
+ Coverage     91.39%   93.12%   +1.73%     
- Complexity       32       39       +7     
============================================
  Files             7        9       +2     
  Lines            93      131      +38     
============================================
+ Hits             85      122      +37     
- Misses            8        9       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@peterfox
Copy link
Collaborator

@Vision42 I was unable to push fixes directly to your branch but I've created a patch file.

cockroachdb-laravel-320d3be-Add mechanism to handle failing with outdated DBAL.patch

What my changes do, is it will allow users to use the package with outdated Dbal installs but it will flag an exception that they should warn they need to update to a new version. For the tests, if Dbal is below 3.5 then those tests will be skipped. This feels like the best way to make users aware that they might have problems if they're using an outdated package without forcing it as Dbal isn't a Laravel dependency and I don't want to make it one for this package either especially as it's no longer used in Laravel 11.

I just want to say thank you for your patience and hard work on this as well. Projects like this one only really work with contributors such as yourself.

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