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

Support DateTime instances #231

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Tigrov
Copy link
Member

@Tigrov Tigrov commented Aug 5, 2023

Support for the time type with test coverage will be added after review of #230

Q A
Is bugfix?
New feature? ✔️
Breaks BC? ✔️
Fixed issues #725

@what-the-diff
Copy link

what-the-diff bot commented Aug 5, 2023

PR Summary

  • Enhanced ColumnSchema class with a new method
    This change introduces a phpTypecast method to the ColumnSchema class, paving the way for a more versatile use of the class in different scenarios.

  • Enhanced Connection class
    We've added a new method initConnection to the Connection class. This would help in setting up the initial configurations for the database connection with ease.

  • Advanced datetime handling
    Through the addition of getDateTimeFormat in the Schema class and modifying the createColumnSchema method, we're now more adept in handling time and date data types. The normalizeDefaultValue modification further optimizes how we handle default values for timestamps.

  • Improved testing
    We're enforcing code quality and reliability by introducing new tests to ColumnSchemaTest and SchemaProvider. These additions ensure our methods for handling different column types and their attributes are thoroughly checked.

  • Updated SQL fixture
    The oci.sql fixture file has seen changes, too, to support columns with different default values for timestamps and dates. This change makes our test data align better with the real-world scenarios.

@codecov
Copy link

codecov bot commented Aug 5, 2023

Codecov Report

Patch coverage: 97.05% and project coverage change: -0.08% ⚠️

Comparison is base (744eb43) 98.29% compared to head (1269d82) 98.21%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #231      +/-   ##
============================================
- Coverage     98.29%   98.21%   -0.08%     
- Complexity      186      194       +8     
============================================
  Files            16       16              
  Lines           585      617      +32     
============================================
+ Hits            575      606      +31     
- Misses           10       11       +1     
Files Changed Coverage Δ
src/ColumnSchema.php 92.85% <83.33%> (-7.15%) ⬇️
src/Connection.php 97.22% <100.00%> (+0.55%) ⬆️
src/Schema.php 100.00% <100.00%> (ø)

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

@Tigrov Tigrov marked this pull request as ready for review August 5, 2023 16:19
Copy link
Contributor

@darkdef darkdef left a comment

Choose a reason for hiding this comment

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

Need restore code coverage

return parent::phpTypecast($value);
}

public function hasTimezone(): bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Need have a tests for public functions

Copy link
Member Author

Choose a reason for hiding this comment

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

ColumnSchema::hasTimezone() can be replaced with two new abstract types SchemaInterface::TYPE_TIMESTAMPTZ and SchemaInterface::TYPE_TIMETZ

Tests will be added after review this option in yiisoft/db #736

parent::initConnection();

$this->pdo->exec(
<<<SQL
Copy link
Contributor

Choose a reason for hiding this comment

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

What benefits in changes settings of db? What about cases with default settings?
This changes can be in documentation only. It's change user experience.

Copy link
Member Author

Choose a reason for hiding this comment

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

When getting datetime values from Oracle they are returned in a specific format that is not suitable for proper parsing by DateTime.
Yes, sure it will need to be added to the documentation

@Tigrov
Copy link
Member Author

Tigrov commented Aug 6, 2023

Need restore code coverage

Yes, mentioned about this in the description:

Support for the time type with test coverage will be added after review of #230

@Tigrov Tigrov marked this pull request as draft August 17, 2023 13:51
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