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

Adds Custom Data Types (Data Domain) #1320

Merged
merged 18 commits into from
Apr 8, 2020
Merged

Conversation

twoixter
Copy link
Contributor

@twoixter twoixter commented Mar 6, 2018

As commented in #753 this PR adds Custom Data Types to phinx.

This feature works by adding a root config option to phinx.yml creating a data domain:

# Add a root config option to "phinx.yml" to start a Data Domain
data_domain:
    phone_number:           # You can add different new data types
        type: string        # Use a base type as reference
        length: 20          # Using default column options
    address_line:
        type: string
        length: 150
    file:
        type: blob          # You can even extend special types
        limit: BLOB_LONG    # For MySQL DB. Uses MysqlAdapter::BLOB_LONG
    boolean:
        type: boolean       # Or customize native phinx types
        signed: false       # Customization of the boolean to be unsigned
    image_type:
        type: enum          # Enums can use YAML lists or a comma separated string
        values:
            - gif
            - jpg
            - png

You can then use the new data types in migrations:

// Use our user defined types from config
$table->addColumn("customer_phone", "phone_number");
$table->addColumn("file_upload", "file", ["null" => true]);

src/Phinx/Config/Config.php Outdated Show resolved Hide resolved
src/Phinx/Db/Adapter/AbstractAdapter.php Show resolved Hide resolved
src/Phinx/Db/Adapter/AbstractAdapter.php Outdated Show resolved Hide resolved
src/Phinx/Db/Adapter/AbstractAdapter.php Outdated Show resolved Hide resolved
src/Phinx/Db/Adapter/AbstractAdapter.php Outdated Show resolved Hide resolved
src/Phinx/Db/Adapter/AbstractAdapter.php Outdated Show resolved Hide resolved
tests/Phinx/Console/Command/ListTest.php Outdated Show resolved Hide resolved
tests/Phinx/Db/Adapter/DataDomainTest.php Outdated Show resolved Hide resolved
tests/Phinx/Db/Adapter/DataDomainTest.php Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Mar 6, 2018

Codecov Report

Merging #1320 into 0.next will decrease coverage by 1.14%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             0.next    #1320      +/-   ##
============================================
- Coverage     76.27%   75.12%   -1.15%     
============================================
  Files            57       35      -22     
  Lines          5740     4825     -915     
============================================
- Hits           4378     3625     -753     
+ Misses         1362     1200     -162     
Impacted Files Coverage Δ Complexity Δ
src/Phinx/Config/Config.php 100.00% <100.00%> (+0.76%) 0.00 <0.00> (-61.00) ⬆️
src/Phinx/Db/Adapter/AbstractAdapter.php 88.99% <100.00%> (+2.32%) 0.00 <0.00> (-33.00) ⬆️
src/Phinx/Db/Adapter/AdapterWrapper.php 66.92% <100.00%> (-1.73%) 0.00 <0.00> (-55.00)
src/Phinx/Db/Table.php 94.76% <100.00%> (-4.12%) 0.00 <0.00> (-62.00)
src/Phinx/Migration/Manager.php 90.34% <100.00%> (-1.58%) 0.00 <0.00> (-149.00)
src/Phinx/Console/Command/Breakpoint.php 36.00% <0.00%> (-64.00%) 0.00% <0.00%> (-15.00%)
src/Phinx/Console/Command/AbstractCommand.php 50.89% <0.00%> (-33.58%) 0.00% <0.00%> (-37.00%)
src/Phinx/Db/Adapter/ProxyAdapter.php 68.75% <0.00%> (-25.85%) 0.00% <0.00%> (-12.00%)
src/Phinx/Db/Adapter/SQLiteAdapter.php 87.64% <0.00%> (-9.42%) 0.00% <0.00%> (-198.00%)
src/Phinx/Console/Command/Init.php 76.59% <0.00%> (-6.02%) 0.00% <0.00%> (-14.00%)
... and 47 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b0eab8...3796795. Read the comment docs.

@dereuromark
Copy link
Member

Whats the status on this?
Please close all conversations that are resolved.

@dereuromark
Copy link
Member

Any chance we can revive and merge this?

@twoixter
Copy link
Contributor Author

Yes @dereuromark , I'll try to resolve all merge conflicts and resolve any conflicts left asap.

twoixter and others added 4 commits March 23, 2020 21:30
# Conflicts:
#	docs/en/migrations.rst
#	src/Phinx/Db/Adapter/AbstractAdapter.php
#	src/Phinx/Db/Table.php
#	tests/Phinx/Db/TableTest.php
@twoixter
Copy link
Contributor Author

@dereuromark I think I've resolved all issues! Please, let me know if there is something still unresolved.

@dereuromark
Copy link
Member

So based on your changes we can also ship the cake core type binaryuuid now?
See #753 (comment)

@dereuromark
Copy link
Member

We are releasing a new major (0.12) soon.
Based on your changes to interfaces ( src/Phinx/Config/ConfigInterface.php ), should we maybe merge this into 0.next branch?

@twoixter
Copy link
Contributor Author

So based on your changes we can also ship the cake core type binaryuuid now?
See #753 (comment)

I'm not sure if this is related. I mean, my changes are related to Phinx migrations on standard Phinx adapters @ src/Phinx/Db/Adapter/*. So I think you'll get the same error as in the #753 comment since it does not automatically integrates custom Cake types.

To add a new binaryuuid for Cake, I think you'll need to create a new data domain in the config file. For example:

# Add a root config option to "phinx.yml" to start a Data Domain
data_domain:
    binaryuuid:       # Create the "binaryuuid" data type
        type: blob    # Use a base type as reference (Is Blob good for binaryuuid?)
        default: null
        null: true

Adding this new "data domain" into phinx.yml allows you use it as a new data type:

$table->addColumn('uuid', 'binaryuuid');

I don't know if it's ok to automatically inject Cake types by default for folks (like me) that uses Phinx outside CakePHP projects.

@twoixter
Copy link
Contributor Author

We are releasing a new major (0.12) soon.
Based on your changes to interfaces ( src/Phinx/Config/ConfigInterface.php ), should we maybe merge this into 0.next branch?

Absolutely, please merge into the most relevant branch. Thanks!

@dereuromark
Copy link
Member

Can you switch the target branch to 0.next?

@twoixter twoixter changed the base branch from master to 0.next March 24, 2020 00:42
@twoixter
Copy link
Contributor Author

@dereuromark I just switched branches to 0.next and then the Travis CI shows many errors from the tests. Did I just done it wrong or those tests are just "work in progress" from the 0.next branch? ;-)

@dereuromark
Copy link
Member

It is possible
If those arent easily fixable, you can also make a changeset with only yours.
And we merge/fix the rest separately.

@twoixter
Copy link
Contributor Author

Hi @dereuromark !! I've reviewed the new errors and I think they're not related to changes from my branch.

They're easy to fix I think, but I find it weird to fix tests on this branch for other not related features.

So, what are the options? I don't know how to make a "changeset" :-D Bear with me, hehehe.

@dereuromark
Copy link
Member

Either fixing the other changes in another PR and we merge that first, then this one will be auto green
Or all in this one.
The result would be the same.

@dereuromark
Copy link
Member

Technically, those are not different features, but as you can see from
https://github.com/cakephp/phinx/branches
You now rebased in a way that it includes a forward port of master into 0.next.
This has to be done anyway, as we need to be up to date with bugfixes before we do
more features and releases on 0.next.

@twoixter
Copy link
Contributor Author

Thank you @dereuromark !! That might be it. Does it makes sense if I try to help merging master into `0.next' then?

@dereuromark
Copy link
Member

master is now merged into it.
Are you able to re-apply your changes?

@twoixter
Copy link
Contributor Author

twoixter commented Apr 8, 2020

@dereuromark Done!

@dereuromark dereuromark merged commit 8c4d5b5 into cakephp:0.next Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants