Closed (fixed)
Project:
Extra Fields Checkout Pane
Version:
6.x-2.0-beta1
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
6 Jan 2012 at 17:07 UTC
Updated:
17 May 2012 at 14:00 UTC
Jump to comment: Most recent file
Comments
Comment #1
igor.ro commentedHere is the patch
Comment #2
megachrizHi igor.ro,
I have never used the Features module. Can you tell me what the code in your patch does and why it should be added to the Extra Fields Pane module?
Comment #3
igor.ro commentedIt allow to export fields as php code, check if field configuration in db differs with code one and revert to code version.
Very usefull thing.
Comment #4
megachrizI set the status to "needs review", so other Extra Fields Pane users know there is a patch here to review. I'll plan to test your patch myself too, but it can take a while because I'm currently busy with getting Ubercart Addresses ready for Drupal 7.
Comment #5
thedavidmeister commentedI'm testing this now. It looks good, at least an export is being generated for me and the format of the export looks like it "should" work. I haven't had to test the import side of things yet, I'll know when we attempt to stage our next release.
Luckily uc_extra_fields_pane uses machine names to keep track of the fields so I'm pretty confident that it will all work as expected :)
Only thing is that "git apply [thepatch.patch]" didn't work for me, I had to use "patch -p1 < [thepatch.patch]" so I wonder if the patch was generated badly somehow? hopefully drush doesn't care :)
@MegaChriz, this is actually quite important functionality to consider including to ensure the long term future of this module. The patch should be ported to the D7 version if possible. Features enables a more professional workflow for Drupal - ie. I can develop using your module locally on my laptop and then export the changes through code to the live site when I'm ready to rather than attempting to re-create the changes manually on a live site which is prone to human error.
One of my clients requires that I don't interact with the database through the UI as they have several domains sharing the same code, I need to be able to make all changes through exported Features or update hooks. Without patches like this, even changes that are minor through the UI become a chore once hook_update_N needs to be included in the workflow. I'm sure there will be an ever increasing number of developers in a similar position to myself as Features transitions from being a luxury to a standardized workflow (something similar to the Features hooks will be included in D8 core I believe).
Comment #7
megachrizI have done some testing with this, but I want to test it some more. I can see the potention of this feature and I will surely add to the module when it's tested enough.
I've seen three things about the patch that should be improved before the feature will be added to the module.
1. The patch adds an implementation of
hook_init()to the module that loads the file uc_extra_fields_pane.features.inc. This means that file will be loaded on every request. This is not necessary. It's better to putuc_extra_fields_pane_features_api()in the module file and point to uc_extra_fields_pane.features.inc in that function. Looking at the Features API documentation, it's possible to declare a file to be included:2. It would be better if the code for features integration makes use of the Extra Fields Pane API, introduced in 6.x-2.0-beta1. This way, modules that implement hooks of Extra Fields Pane have a chance to add their data or to react upon cases when a field is loaded, inserted or updated.
3. I think the feature should be grouped in a group called "Ubercart" instead of "Features". Also, instead of "Ubercart fields field" I think "Extra Fields Pane field" is a better name.
Comment #8
thedavidmeister commentedthe other patch failed for me and i can't build if it's in a makefile. Even if it isn't perfect for all the reasons outlined by MegaChriz I'd like a re-roll so I have something to work with.
Comment #9
megachrizI have done some additional testing with the patch yesterday and I changed it as noted in #7. It looks okay to me, but as I'm new to the Features module I'd like some feedback before I definitively add it to the module.
@thedavidmeister, igor.ro
Can you verify the attached patch still works as desired?
Comment #11
megachrizI don't know why that test is failing. The tests pass locally and the noted exceptions seems to have nothing to do with the patch. Also, the errors seems to happen in Ubercart and not in Extra Fields Pane. Setting back to "needs review".
Comment #12
megachriz#9: features-integration-1396532-9.patch queued for re-testing.
Comment #14
megachrizHm, that didn't work. It looks like qa.drupal.org runs this patch in a bit different environment than when testing only the branch.
@thedavidmeister, igor.ro
Do you want to test the patch anyway and report back if it still works for you?
Comment #15
igor.ro commented@MegaChriz Ok will do tomorrow.
Comment #17
thedavidmeister commenteduh, I'm on two hectic deadlines this fortnight... it looks unlikely that I'll get a chance to test anything here in the immediate future :(
Comment #18
megachrizI haven't found any problems with the Features integration code. The patch in #9 is committed.