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

Refactoring of AbstractSchema::getColumnPhpType() method #728

Merged
merged 2 commits into from
Jul 24, 2023

Conversation

Tigrov
Copy link
Member

@Tigrov Tigrov commented Jul 22, 2023

Q A
Is bugfix?
New feature?
Breaks BC?
Fixed issues -

@what-the-diff
Copy link

what-the-diff bot commented Jul 22, 2023

PR Summary

  • Refactoring in Changelog
    Some minor modifications were made to the Changelog file. The notable changes are regarding the typecast and AbstractSchema refactoring which have been improved for better functionality.

  • Modifications to getColumnPhpType method
    In the AbstractSchema file, a particular method called getColumnPhpType has undergone changes. It now uses a 'match expression'- an upgraded feature that makes the code cleaner and easier to understand.

  • Handling different PHP_INT_SIZE and isUnsigned flag
    Part of these changes within the getColumnPhpType method ensures that the method can accommodate different PHP integer sizes and handle the isUnsigned flag, improving the software's versatility and robustness.

  • Addition of a default case
    A new default case has been incorporated in the method, which returns a string as per the SchemaInterface, providing a baseline response should unexpected situations arise. This makes the system more fault-tolerant.

@codecov
Copy link

codecov bot commented Jul 22, 2023

Codecov Report

Patch coverage: 88.88% and project coverage change: -0.10 ⚠️

Comparison is base (13ddab5) 99.83% compared to head (baeb0b1) 99.73%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #728      +/-   ##
============================================
- Coverage     99.83%   99.73%   -0.10%     
+ Complexity     1272     1269       -3     
============================================
  Files            63       63              
  Lines          3040     3038       -2     
============================================
- Hits           3035     3030       -5     
- Misses            5        8       +3     
Impacted Files Coverage Δ
src/Schema/AbstractSchema.php 99.42% <88.88%> (-0.58%) ⬇️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Tigrov Tigrov marked this pull request as ready for review July 22, 2023 03:24
// abstract type => php type
SchemaInterface::TYPE_TINYINT => SchemaInterface::PHP_TYPE_INTEGER,
SchemaInterface::TYPE_SMALLINT => SchemaInterface::PHP_TYPE_INTEGER,
SchemaInterface::TYPE_INTEGER => SchemaInterface::PHP_TYPE_INTEGER,
SchemaInterface::TYPE_BIGINT => SchemaInterface::PHP_TYPE_INTEGER,
SchemaInterface::TYPE_INTEGER => PHP_INT_SIZE === 4 && $column->isUnsigned()
Copy link
Member

Choose a reason for hiding this comment

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

This behavior is for all dbms or is specific, i find it strange.

Copy link
Member

Choose a reason for hiding this comment

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

In PHP for 32bit system maximum integer number is 2147483647. Seems, in most (maybe in all) DBMS unsigned int value will be more (for example in MySQL — 4294967295).

Copy link
Member

@vjik vjik left a comment

Choose a reason for hiding this comment

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

Please, add line to changelog

@samdark samdark merged commit 669fca4 into yiisoft:master Jul 24, 2023
74 checks passed
@samdark
Copy link
Member

samdark commented Jul 24, 2023

👍

@Tigrov Tigrov deleted the refactoring_getColumnPhpType branch August 4, 2023 08:59
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.

4 participants