Skip to content

Content Type Manager: update_if_exists option#123

Closed
blankse wants to merge 1 commit intokaliop-uk:masterfrom
datafactory:update_if_exists
Closed

Content Type Manager: update_if_exists option#123
blankse wants to merge 1 commit intokaliop-uk:masterfrom
datafactory:update_if_exists

Conversation

@blankse
Copy link
Contributor

@blankse blankse commented Sep 8, 2017

Adds a update_if_exists option to the create mode.

It is helpful if you are not sure if the content type already exists. E. g. for the first migration.
It could be the install of the bundle or already a update.

@gggeek
Copy link
Member

gggeek commented Sep 19, 2017

Hello.

I am not sure that I get exactly the need / what the bundle is supposed to do.

As far as I can tell from your patch, all it does is to allow a graceful failure in case you want to create a ContentType but it already exists.
In case that the given CT already exists, no attempt is made to check that it actually conforms to the version that is described in the migration yaml.
I find the latter a big limitation, as there is the risk of junior developers being surprised by the latter case.

On the other hand we could try to go instead for a so-called 'upsert' operation, ie. if the CT does not exist create it and, if it exists, make it comply with the yml.

The problem with this second option is that it is very hard to start from a single yml definition and make it work for both the create and the update case. I have already tried to make it work (for Content, not ContentType, admittedly) in another project, and I had to come up in the end with an hybrid piece of yml that was not very beautiful to look at.

As a side note, I try to make this extension be as 'generic' and 'uniform' as possible, meaning that any new yaml flag or operation should be, if possible at all, supported by all executors, not just one.
Which means that, if we want to add not proper 'upsert' support, but simply 'create-if-needed' support, I'd like to add it everywhere, not just for ContenTypes.

What do you say ?

@gggeek
Copy link
Member

gggeek commented Sep 19, 2017

In fact I think that it is already possible to achieve what you need, albeit in a more convoluted way:

-
    type: content_type
    mode: load
    match: ...
    references:
        -
            identifier: count
            attribute: count
-
    type: migration
    mode: cancel
    if:
        "reference:count":
            eq: 0
-
    type: content_type
    mode: create

You could then add a 2nd migration, with the opposite guard condition, that applies an update

@blankse
Copy link
Contributor Author

blankse commented Sep 21, 2017

Until now we managed our content types with the ezxmlinstaller extension.
There it was possible to use the "extend" action. Which create oder update the content type.
Now it is possible that some content type exists and other don't exist.

With my option or with a upsert mode I can put all in one migration.
At the second approach I must create two migration for each content type (about 40). The MigrationVersions folder will be large at the beginning...

I would create a upsert function in the ContentTypeManager class and check if exist and call create or update. I think this function would make sense for other executers.

What do you say?

@blankse
Copy link
Contributor Author

blankse commented Sep 22, 2017

Here my thought in a own PR: #127

@gggeek
Copy link
Member

gggeek commented Sep 22, 2017

A couple of points worth discussing before we try to implement it:

  1. usage of 'identifier' vs 'match' conditions for matching the CT to be modified. Do we allow only one or both? If we only allow identifier, should we check that 'match' is not set to avoid confusion in users ?

  2. handling of extra fields in the CT: what if the CT already exists and has fields A,B,C, but the YML has A,B,D: shall we drop field C? Shall we let the user choose, via an extra option, whether to keep extra fields or not?

@blankse
Copy link
Contributor Author

blankse commented Sep 28, 2017

Close in favor of #127

@blankse blankse closed this Sep 28, 2017
@blankse blankse deleted the update_if_exists branch October 24, 2017 10:57
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