Needs work
Project:
Multifield
Version:
7.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
1 Jun 2016 at 07:35 UTC
Updated:
29 Jul 2016 at 01:53 UTC
Jump to comment: Most recent, Most recent file
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | 2738313-convert-code-reusable-testable.patch | 35.17 KB | dave reid |
| #2 | 2738313-convert-code-reusable-testable.patch | 24.86 KB | dave reid |
Comments
Comment #2
dave reidComment #5
dave reidWithout the missing class file now.
Comment #8
elijah lynnWow, that is quite the patch, much more thorough than I was going to make at a first pass!
Before this patch, the convert-field-collection command used to rename the field to the same name. If I run it now with no arguments and select a FC or with a FC argument it says 'the target field FIELD_NAME already exists'.
Comment #9
elijah lynnOkay, I see now that the way the old code migrated fields, it eventually deletes them when cron runs over time. So maybe that is why you have in the new code to not allow the field name to be the same?
We have a ton of instances where the field collection API was used for attaching field collections to nodes on bulk operations and other CRUD operations, so I was trying to avoid having to change the field names, in addition to reexporting features etc.
If you see this and can verify any of what I just said that would be awesome!
Comment #10
elijah lynnI am actually getting errors like this on cron, notice the 0 after the field table name. I believe it has something to do with these image fields not actually having the images present before I do the conversion, since these are development environments. There is also the possibility you are testing with the latest Field Collection whereas we are on beta8 and the latest release is beta11, + we have 3 contrib patches applied to beta 8. Still investigating but thought I would post this in the meantime:
update: This comment is getting this error pre-patch above.
Comment #11
dave reid@Elijah It definitely isn't the intent to prevent creating a multifield with the same field name. That sounds like a bug in the above code that I would want to address/fix.
Comment #12
elijah lynnThanks Dave,
I was able to remove $source_field_name for now and even though it didn't show correctly in the drush pre-conversion message this did the trick, because if we don't pass in the target, then target is the source and there is a check in L49 of the __construct that also sets this if target is empty, which it is not at that time and then throws the exception that the target already exists.
Still testing things right now, will try to post a patch back in a bit. The new patch gets rid of the error I was having on cron and also appears to not be deleting images on cron anymore.
I do have a question thought about the file_usage table, the image files all say they belong to field_collection still. Should we try to update that as well?
Comment #13
elijah lynnThis is working well in an update hook but I think my implementation may be off because I cannot immediately run field_purge_batch() afterwards nor in a subsequent update hook. I keep getting an error about 'PDOException' with message 'SQLSTATE[42S02]: Base table or view not found: 1146 Table 'loc_example.field_data_' doesn't exist'. Now, if I run the conversion update hook fully, then uncomment the subsequent field_purge_batch update hook then it does work fine. But just not in one swoop. I also tried drupal_flush_all_caches() thinking that had something to do with it. But what I really think is that my implementation is incorrect. I am batch_set()ing right inside the update hook, which is a batch itself.
So I started going down the route of using $sandbox and the functions inside $converter->getBatch() and the private properties and 5th, required $context argument at the end of convertFieldValues() which I think gets passed automatically by the batch API, led me to believe maybe there was a way of implementing this that I don't know about yet.
Could you provide an example of implementing this in an update hook?
Comment #14
elijah lynnTo clarify, this worked well except I cannot run field_purge_batch() right after it or in a subsequent update hook during the same
drush updb:Comment #15
elijah lynnYeah, so if I batch_set() inside the update hook it actually finishes running the update hook pretty quickly and then goes to the next one right away. And then after that it is processing the conversion batches (as evidenced by CPU usage and a long wait). So it is definitely a timing issue if I use that method. I looked for examples of using batch_set() inside of *.install files and found nothing so I think there must be another way.
Comment #16
elijah lynnI should add that I tried batch_process() in the update hook but ran into some errors that I don't have at the moment, but now I am pursuing it again because that would make sense to process it right away.
Comment #17
elijah lynnYeah, if I do a batch_process() inside the update hook I get a
PHP Fatal error: Maximum function nesting level of '256' reached, aborting! in. Here is the full code I tried: