Closed (won't fix)
Project:
Drupal.org security advisory coverage applications
Component:
module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
10 Jul 2012 at 19:16 UTC
Updated:
15 Jul 2012 at 08:12 UTC
Jump to comment: Most recent file
Comments
Comment #1
FranciscoLuz commentedComment #1.0
FranciscoLuz commentedAdded the git command to download the module.
Comment #2
parwan005 commentedHello,
Did manual review of your code.
My suggestion would be to use php_eval rather than eval. As we should use drupal functions and drupal_eval has been replaced by php_eval in D7. Please refer http://drupal.org/node/224333#php_eval
Please work on your ventral error report
http://ventral.org/pareview/httpgitdrupalorgsandboxdrupalista-br1661818git
Comment #2.0
FranciscoLuz commentedCorrected the git command.
Comment #2.1
FranciscoLuz commentedAdded list of other projects reviewing done by me.
Comment #3
FranciscoLuz commentedphp_eval was actually the first thing I used but I couldn't get it to work.
You can do the experiment yourself, first add the following code into #ajax attribute at the field widget form:
Then change these lines:
to
The ajax module will give you a nasty error.
There are no errors in ventral but warnings. The warnings are related to code comment lines that spans over 80 characters. Those lines are URLs for @see.
Thank you.
Comment #4
FranciscoLuz commentedTagging PAReview: review bonus.
Comment #5
pjcdawkins commentedManual review.
Looks good. It seems to solve a common problem in quite an elegant way.
I got it to work and it seems quite intuitive (to a Drupalist). I added a "Full name" field to the Basic page content type, with subfields "First name", "Middle name" and "Last name". Screenshot of result attached.
A few things:
1) Presumably #3 is because as the docs for
php_eval()state, you need to surround code in <?php ?> tags to use it (unlikeeval()). You could do that just before running the function, although I'm not sure why it's preferred to eval().2) In a manual review, I started to add a Composed Field to a content type, and entered the number of sub-fields in the box and "hit the tab key" as you suggest. That gave an error:
Notice: Undefined offset: 2 in _composed_field_build_vertical_tab_elements() (line 401 of <code>sites/all/modules/composed_field/composed_field.module)3) When the vertical tabs are built (successfully despite the above notice), and I wanted to add a #title for each of my sub-fields, I clicked on the #title vertical tab, then checked "Enable #title attribute for Subfield 1". The AJAX throbber whirred and then, a little bit frustratingly, the #type vertical tab was selected again. So I go back to the #title tab, enter my first subfield title, and then check "Enable #title attribute for Subfield 2". And of course the #type tab gets selected again.
Is there any way to prevent this behaviour? Once the vertical tabs are reloaded via AJAX (as I guess they are) can you use a bit of jQuery to select the tab I was previously looking at?
(This happened for all subfield attribute tabs)
4) In the #required tab, when I select "Enable #required attribute for Subfield 1", as well as negotiating problem (3), I also have to select "Make required". That seems like too many clicks. Could it just be a "Make Subfield 1 required" checkbox?
Comment #6
pjcdawkins commentedSorry forgot to set status. Needs work (at least because of the 2nd problem I mentioned in comment #5).
Comment #7
FranciscoLuz commentedNope, that's not the case. In this particular case, no clue why, php_eval just does not work.
I could not reproduce that. Yes, my php.ini
error_reporting = E_ALL | E_STRICT(Show all errors, warnings and notices including coding standards.)Just make sure your caches are cleared and let me know if you or anyone else still get that.
Yes, I find that annoying too. To be very honest, I don't know how to implement it.
As you had pointed out this will need some jQuery. I opened a feature request issue here #1682002: Keep the tab selection focused on the same tab that was selected before the ajax rebuilding. If you or anyone else could spare some time helping me out on this one I would very much appreciate it, and also buy you a cup of coffee.
EDITED: I fixed it with no need to implement any jQuery.
Done, fixed it.
Comment #8
klausiProject 1: http://drupal.org/node/1678380
Project 2: http://drupal.org/node/1246104
As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).
If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.
Comment #9
FranciscoLuz commentedComment #10
klausimanual review:
This module looks like a duplicate of field_collection: http://drupal.org/project/field_collection . Please take a look at that code and check the embedded widget type. We prefer collaboration over competition to avoid duplicated efforts and confused users that don't know what to use. Please get in touch with the maintainers to discuss how field_collection could be improved to fit your use case.
If that fails for whatever reason please set this back to "needs review". Thanks!
Comment #11
FranciscoLuz commentedComment #12
FranciscoLuz commentedThe Field collection uses a different approach for creating composed fields. Here are the main differences between this module and the "Field Collection":
The host field created by "Field Collection" is in fact an entity that gets attached to the fieldable entities (nodes, taxonomy, comment, commerce products, etc). So, although you attach a Field Collection through the Field UI, what is being attached is not a field but an entity.
Once you get your new entity (host field) attached to your fieldable entity (node, comment, taxonomy, commerce product, etc) then you have to attach the fields ( what would be the subfields ) into the host field ( Field Collection entity).
Each subfield is actually a field on its own, that is, from the database point of view, each subfield is a single field.
"Composed Field" on the other hand attaches a field (host field), not an entity, to your fieldable entities (node, comment, taxonomy, commerce product, etc) through the Field UI. At the end of the day, even if you set a field with say 4 subfields, you will end up having only one field and a single value saved into the database.
Unless you have a very specific use case need "Composed Field" should do what you need with less effort and less resources.
I have added the above information into the Composed Field page.
Although Field Collection and Composed Field can produce the same result, these 2 modules are fundamentally different and I do not see competition going on here.
To avoid controversy on that interpretation I have switched my attention back over to my old application at http://drupal.org/node/1246104
Comment #12.0
FranciscoLuz commentedReviews of other projects