Posted by davidwhthomas on November 17, 2009 at 7:38am
| Project: | Content Construction Kit (CCK) |
| Version: | 6.x-2.x-dev |
| Component: | content_copy.module |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (won't fix) |
Issue Summary
The current behaviour is for content_copy to skip a field if it already exists in the system.
The attached patch uses content_field_instance_update to update the field instead of skipping it.
This is useful for our deployment environment as content_copy exported definitions are kept in source control and migrated between development and production sites.
Allowing the updating of widget settings on existing type is therefore a useful feature.
Patch attached.
DT
| Attachment | Size |
|---|---|
| content_copy.module.patch | 892 bytes |
Comments
#1
I'm tempted to say won't fix here. In my understanding, updating fields was not implemented because it is not easy as it seems. It is not only the definition of a content type, but user data what matters here as well, and please note there are a few things you cannot do from the UI when it comes to updating fields, so a mass update operation should ensure the same exact precautions are taken into account.
I'm setting this issue as needs work, but I still think this is won't fix. As an alternative solution, one can create a script to mass import/update field definitions. The API is there (content CRUD), and the logic to do it is not so complex, as it can be seen in Content Copy module. But, IMHO, CCK cannot offer such a function because if it is not possible to ensure damage to data is not possible, and that should be 100% assured, then it's a no no.
#2
patch re-rolled for 6.x-2.x-dev
#3
#4
Thanks for the reply.
I believe given that the small patch uses the content.crud.inc api function:
content_field_instance_updatevscontent_field_instance_create, the existing field update would be taken care of comprehensively in a similar way to which the new fields are created from their generated definition.The only addition I could think of is perhaps a checkbox on the import form to select skip or update, otherwise I can't see any reason not to use it. If I'm importing a content type, I'd expect the field setting to be updateable also.
I've tested it on various field types and it works fine.
If it's too difficult, I'll consider creating the patch as a small contrib module though it would be better placed in content_copy. I'd be interested to hear what others think also.
DT
#5
@markus_petrux Would you be interested in a patch that added the update/skip as an option with a checkbox on the import form? That way, the user could make the choice which option to select when importing a content type.
#6
You can use http://drupal.org/project/cck_sync for CRUDing (i.e. also updating exiting fields)
#7
@Amitaibu: Thanks for that module.
@davidwhthomas: IMO, this is not as easy as it seems, as per my comment in #1. Working on a patch for Content Copy shipped with CCK means a few things need to be taken into account, and then it needs testing and feedback from as many sources as possible. So, I would suggest, to resolve your particular need, to a) explore CCK Sync module, or b) implement this feature yourself in your module using CRUD provided by CCK.
#8
@Amitaibu, thanks for the link, that looks like an interesting project. I might try it out in the future and possibly submit some patches there if that's ok. I'm currently using a wrapper module around the cck export/import we use which has some useful functionality there, e.g export all types to file and select from that file list which to import/sync.
@markus_petrux, thanks for your replies, I'm still using the patch and it's working fine here, as soon as there's any problem with it, I'll look into cck_sync instead. I might need to implement is as a separate module however, for ease of update.
#9
Hello. My issue has been marked as duplicate of this one, so I'll keep discussing it here.
Just a note: the patch I attached to the issue linked above also provides the deletion of fields no longer existing in the new content type definition. I am attaching the patch here as well, but bear in mind it has been built against CCK 6.x-2.6.
The main reason that led me to do the patch is for deployment reasons too: it makes the process I described in this article working 100%.
The point here is that CCK content types should provide more features, not less. A regular Drupal content type is defined in code, and then installed. Afterwards, you can change it by means of the hook_update_N: that means you can alter its structure by updating, adding and dropping fields.
I don't see why one shouldn't be able to do the same with CCK content types, also considered that it is possible, because the APIs to do that are there.
It has been said that "we should take into account some other things in order to do this properly", but so far no one said exactly what.
markus_petrux said:
Fair enough. It also true that you always update a field through the UI, and you export it.
So, I don't see any difference between a field A updated via UI, and the same field A updated (perhaps somewhere else) via the code exported as result of that very same UI operation.
Plus, our patche(s) use the
content_field_instance_update(andcontent_field_instance_deletetoo in my case), which are the same APIs called by the UI when the user triggers the related operations.markus_petrux also said:
Fair enough, again. This doesn't mean we should neglect the patch, just spend more time testing it.
About CCK_sync. Good to know about it, but for the time being I'll be sticking to my solution: I built a simple module where I copied the "patched" version of the
content_copy_import_form_submitto a new function, and then I implemented thehook_form_alterto change the submit handler of the Content Copy import form to this new function.Once again, I strongly believe that this behaviour should be available out fo the box.
Thank you all for the stimulating discussions.
Best regards,
Vincenzo
#10
#11
Patches in unified format, please.
Also, this is not acceptable because we are not performing the same kind of validations that are enforced using the UI. CCK is used by many, many sites, and we cannot deliver a tool that is going to cause problems when a field of one type is updated with another type, a field is moved to a fieldgroup that does not exist, a field is moved out of a multigroup when it really shouldn't, etc.
I'm tempted so say won't fix again, because I think CCK is not ready to provide this kind of functionality. Otherwise, it would have been implemented at first. It would have been obvious, but it wasn't for some reason.
#12
Right, sorry about the patch format. Attaching the unified version now.
I understand your concern as CCK is very popular, and also I understand if the patch cannot be accepted as it is now, but I believe we can end up with an acceptable version. Maybe you can point me in the right direction to understand which validations I might need to enforce. I am willing to keep working on this to make it reach an acceptable status.
Then again, however, I still have some observations to do. First, I want to remark the one I've already made: the modifications usually, if not always, happen via UI. Which implies that those validations we are talking about are always performed. Once you export the code to import it somewhere else, that code can be considered validated, as it results from a series of validated UI actions.
Secondly, things like moving fields among groups already work without this patch, if I am not mistaken. Anyway, I don't have any problem in that specific case.
Also, If we look at the regular Drupal content types, we can see that we might incur in the same problems you are trying to avoid here. Which leaves me with a simple comment: since who manages the code (either the regular drupal node types or the code exported/imported via CCK content copy) can be considered a developer, if any of the problems you are worried about happens, it is a developer's fault, not a CCK fault. Indeed, this is the same situation where a developer mistakes at changing a regular Drupal node type.
The same holds if you assume that there might be people who dare to edit a CCK content type by editing the exported code (I don't think anyone actually does that). One note here: as far as I know, this would be the only way to actually change the type of a field. The UI does not allow you to do that. And if you do that by hacking the exported code, well, it's not CCK fault, is it? However, we can still think to prevent the field update if we detect a type change.
Anyway, what I am saying here is to take this patch seriously, because the lack of this behaviour in the import process is becoming more and more serious, as Drupal it's being used more and more as framework subject to multi-stage deployment. I know one can work around it, like I did, but I'd rather prefer to have the feature out of box.
Thank you.
#13
Subscribe
#14
I think the option to update/delete fields should be provided to the _user_ who can then choose which action to take.
Rather than leaving it out from the code altogether because of possible real/imagined conflicts, give the user the choice to update/or delete fields on import. A disclaimer, warning can be added if needed.
The field update/delete behaviour doesn't need to be mandatory but could be controlled by a couple of checkboxes on the import form.
Having said that, the cck_sync module looks like a good option for users wishing to update/delete fields on import.
cheers,
DT
#15
I disagree on letting the users decide what to import. The reason is simple.
If I develop a content type (and this actually holds for anything else), I do that with a specific reason in mind. All the subsequent modifications have also a reason to be. If I let other people to choose what to import, all those people, after importing the update, might end up with a different structure despite they will have the same version installed. That's simply called an inconsistence.
It's like I develop some changes to any other piece of software, and let's say that into the version 2.0 I add a number of options; then I let the final users to choose which of those new options they actually want to import. They are going to end up with different 2.0 versions. A non-sense.
Given that, I agree there are workarounds for this, like the one I made or the cck_sync module. However, for sake of consistency, a CCK content type should be fully updatable out of box.
Cheers.
#16
@Vincenzo: I've been thinking a bit more about your particular use case. And I'm now more sure this is won't fix. Let me explain...
If you develop a module that implements a node type, that also includes CCK fields, you need to work with Schema and Database APIs to manage your own tables, but you need to work with CCK CRUD API to manage your CCK fields. For example:
a) If you need to alter the table related to the node type, then you alter the schema and implement a new hook_update_N() function to allow your users to upgrade.
b) This is the same, if you need to alter CCK fields. You need to adjust the .txt file where your exported content type is (equivalent to altering the schema of regular tables), but you also need to implement a new hook_update_N() function to allow your users to upgrade, and here you can use CCK CRUD API to do whatever you need.
You cannot simply rely on Content Copy module to do what you would have to do in your hook_update_N() implementation. See how CCK sub-modules that implement fields update their own field definitions in their own hook_update_N() implementations. They do not use Content Copy. They use CRUD directly.
The reason in Content Copy is not able to deal with all this stuff, and it is not even optimized for that. Content Copy could be used by anyone to do whatever the module allows people to do, and that should ensure no major headaches are caused because something not really supported was done using this tool. At least, I do not wish to see such reports in the CCK issue queue. Hence, this is won't fix unless someone is able to figure out how to be sure the same exact verifications performed through the UI are implemented, in all cases, and this includes support for all possible hook_alters that are possible using the UI, which is something many external modules use to tweak the way CCK works. I think this is close to impossible.
Also, we're living a moment where all big features should be developed with D7 in mind. That probably means something should be done first for Fields in D7, and if so, backported to CCK for D6.
The alternative is proceed as I described in b) above, which is how modules implementing CCK fields deal with changes to their definitions in their own upgrade paths.
#17
@markus_petrux: Well, what can I say, fair enough. At the end of the day, the final choice is up to you. However, let me write down a few more comments before "I go" :)
Firstly, you shouldn't think to my personal use case. I am not doing that. It is true that for me all started from there and that still matters, but I've abstracted things away long time ago now. What you suggest for my personal use case makes sense. Fine. The problems are:
I can still agree that in my programmatic way I should use CRUD directly. This does not change that content_copy should update and drop fields. It's not a matter of how you do something. It's a matter of what you do. And content_copy is not consistent in its current behaviour.
You also said that Content Copy is not able to deal with this stuff. So far, however, I can't see anything that actually confirms that. I can probably see many things that actually disprove that sentence, as my understanding is that I am not the only one using this modified import procedure, and no one has got problems (as far as I know).
Also, I've been thinking of your idea to enforce all the validations in the import procedure. And the reason why you say "this is close to impossible", it's simply because it is wrong by principle. The import function should assume that the code it reads is correct (i.e. validated). This will be, in fact, true, as the code will be generated after the content type has been modified via UI. And the UI enforce all the validations in place. Any misuse and manual manipulation of that code it is not something that Content Copy should care about.
From my perspective is, then, well safe to have this behaviour in the import procedure. However, I understand your concern, as CCK is widespread almost as the Drupal core itself is.
Also, I could agree that we should focus the efforts on Fields-D7, and then, maybe, backport. So, I'll do that. Expect a patch from me about it (yeah, exactly, I don't give up very easily :).
Thanks for your time, anyway.
Cheers,
V.
#18
Re: "The two import procedure (UI e programmatic) will then be different, creating then inconsistency. Inconsistent is never good."
This is not completely accurate. You can export a content type from system A, then you can import that content type to system B, and you'll get 2 exact copies, as far as requirements are met of course, which is something that Content Copy itself does not completely cover, BTW.
However, we're talking about a different use case here. We're in the land where you have already used Content Copy to import content type X from system A to system B, but then you have modified that content type in system A, and now you need a reliable method to replicate those changes. This is a different thing that should be handled differently, and we're here walking the same exact road as when you need to replicate changes to any other table your module implements. You need to implement hook_update_N() for that. That's the way how Drupal works. So, we need to apply the same pattern to CCK fields as well. So, you should implement hook_update_N() to replicate changes in your content type. The same way you should update the original database schema for regular tables, you should also update your exported version of the whole content type so that (both things) targets new installations. But still, you need to provide upgrade paths using your own implementations of hook_update_N().
And so, you have the chance to provide upgrade paths for changes to CCK fields in your content types, the same way you can do this for changes to regular tables. With the later you use Schema and Database API, with the former you should use CCK CRUD API.
One pattern is consistent with the other. It is just you need to use different APIs.
#19
Regarding the programmatic way: using CCK CRUD API in the hook_update_N there where I would have been using Schema API and SQL queries for regular Drupal content types, it's something I can live with, because it makes sense "in a Drupal way".
I still believe the approach in my patch can be safe and it would make much easier updating CCK content types programatically, and it would allow to have a fully functional update procedure from the UI too. Anyway, let's forget about it, for now, as I want to study Fields in D7 before going any further with it.
However, I still see a simple inconsistency in the Content Copy import. A different one, though, much more related to the UI rather than the actual import procedure.
The module behaves in a hybrid way. It imports fields if you choose an existing content type as destination, but it imports content types if you choose to create a brand new content type instead.
The inconsistency arises from the interface not communicating this behaviour clearly, in conjunction with the fact that code imported/exported is always the same, i.e. it always brings with it the content type definition in addition to the fields definition. However, that information is or is not used depending on which kind of import you are going to perform. The point is that the "type" of import is not a user choice, but it is inferred by a user choice. Tiny but substantial difference.
#20
@davidwhthomas I came up with the exact same solution to this problem you have, but I am a little afraid to do this now that I have read this thread. content_field_intance_update() seems safe enough, but have you had any problems on your system while you have been using it in this way?
#21
@mdorrell.
It's probably OK to use content_field_instance_update
I'm actually using the Features module to manage such things these days which appears to handle the variety of content field updates ( add, edit, delete ) no problem.
Perhaps Features could be a good option for you too.
http://drupal.org/project/features
regs,
DT