Provide original entity in hook_field_attach_validate()

Edit: I think I misunderstood what you were saying. You are suggesting that the entity be in $entity->original before it gets passed on to field_attach_validate()?

Just noticed that when editing user profiles for existing users, the uid isn't even in the entity during field validation. So the example in the original post won't even work in all cases, because the entity object is not necessarily complete.

Purely cosmetic markup change.

Tagging.

Added note that it doesn't even work for users; updated with new recommended solution.

I finally got around to opening #1261310: uid is not available in hook_field_attach_validate() for #4.

Added clarification as to why this is not a true entity property addition.

By the way, I think that $form_state['build_info']['args'][0]; is not a good solution--or at least needs a better comment--but unsure yet what a better fix might be.

I think this is a pattern that could hold us over until we (hopefully) have some standardized entity form handling from some of the core initiatives. It's also something we could build on to fix #1261310: uid is not available in hook_field_attach_validate() (by merging in the default values from the entity, thereby providing the uid in the field attach hooks). Certainly it's a lot better than using some bizarre, obscure thing from the build args. :) Interested to see what sun thinks.

Updated issue summary.

For D7, however, we probably want to stay consistent with current $form_state keys; i.e., 'comment_original', etc.

Maybe this plus something like in #1170198: The build variable name for the entity should be the same regardless of type of entity.? Or is that too much redundancy?

Here's a D8 version that just renames the original entity to $pseudo_entity->original per #14.

Couple of potential followup issues:

1. I noticed user_register_form() and user_account_form() use $form['#user'] instead of $form_state['user']; perhaps we should add $form_state['user'] and the same note about deprecation as in user_profile_form. Could be a good novice issue. Edit: #1267978: Clean up use of $form['#user'].
2. Could also open a followup issue renaming stuff to $form_state['entity'] everywhere in D8; not sure if that would be worthwhile or not as a first step toward generic entity form building. Ideally I think that some generic base class would provide a $entity->form() or suchlike.

D7 variant that uses the pattern $form_state['entitytype_original'] as per #14.

Edit: just realized we need to check whether this thing is empty before setting it or it will cause notices for contrib/custom entities.

Checking that the original entity is there before setting it.

D8 version that also checks if the original entity is non-empty.

For term (in patch #19), I believe the entity_type being passed should be 'taxonomy_term' so you have $form_state['taxonomy_term_original'] = $term; based on what I see in http://api.drupal.org/api/drupal/modules--taxonomy--taxonomy.admin.inc/f...? Other than that, the backport looks great. The D8 patch also looks sound.

Hmm, this would put us in the same situation as in #1170198: The build variable name for the entity should be the same regardless of type of entity., if the entity type is taxonomy_term and the form component is $form_state['term']. So we either need that extra, redundant key as in #1170198: The build variable name for the entity should be the same regardless of type of entity. that indicates what the name of the entity form component is, or to just go with the generic entity_original in D7 as well.

@pav: The messages you got indicate the patch needs to be rerolled. I'll do so. And then do so again after #22336: Move all core Drupal files under a /core folder to improve usability and upgrades. :)

Failed hunk was due to entity code being moved into its own module. Patch below fixes this.

Hmm, I was able to apply #19 to the 7.x branch with just offset. In any case, here's a reroll. Note that it is against the current 7.x branch, which means it's for the 7.x-dev version of Drupal.

Is there an issue for the module you mention that references this one?

The patch was created with git. Place the file in the root of your Drupal installation and use the -p1 option:
patch -p1 < form_orig_entity_12220212_25-d7.patch
See also: http://drupal.org/patch

Also, if you are testing the patch on D7, please make note of the questions in #21 and #22.

Rerolled for core/ directory.

Regarding #33, you'll probably want to file that in the queue for that OG module. :)

Related issue in TAC:
#1362210: Notice: Undefined property: stdClass::$name in taxonomy_access_field_attach_validate() of taxonomy_access.module (line 299)

Needs another reroll, I guess. Tagging novice for that task.

Thanks @oriol_e9g!

Still needs an automated test that takes advantage of this property for various entity types.

@BrightBold: The issue is against 8.x, but it is tagged "needs backport to D7." This means that, if the patch is accepted, a D7 patch will be created and committed once the D8 change is finalized.

Thanks @adharris! The test coverage there looks correct and thorough. I reviewed and have one remark about the placement of the tests, plus a few minor points for cleanup.

1. +++ b/core/modules/field/tests/field.testundefined
   @@ -637,6 +637,11 @@ class FieldAttachOtherTestCase extends FieldAttachTestCase {
   +  function setUp() {
   ...
   +    parent::setup('comment', 'taxonomy');
   

   Hmm, I'm not sure it's wise to add this to an existing class. I think we should either create our own subclass of FieldAttachTestCase, or place the tests in comment.test, taxonomy.test, etc. (The latter is probably more correct, since we are relying on specific implementations provided by these entity modules rather than the base entity class... but of course it's less practical. And also I could be totally wrong, so maybe let's go with the simpler option for now.)

2. +++ b/core/modules/field/tests/field.testundefined
   @@ -637,6 +637,11 @@ class FieldAttachOtherTestCase extends FieldAttachTestCase {
   +    // Comment and taxonomy are required to test hook_field_attach_validate.
   
   @@ -948,7 +953,58 @@ class FieldAttachOtherTestCase extends FieldAttachTestCase {
   +      // The field_test module uses drupal_set_message to indicate that the
   

   drupal_set_message() and hook_field_attach_validate() should have parens in the comments here to clearly indicate that they are function names.

3. +++ b/core/modules/field/tests/field.testundefined
   @@ -948,7 +953,58 @@ class FieldAttachOtherTestCase extends FieldAttachTestCase {
   +   * Test hook_field_attach_validate()
   

   Should begin with "Tests" (rather than "Test") and end in a period. Reference: http://drupal.org/node/1354#functions

4. +++ b/core/modules/field/tests/field.testundefined
   @@ -948,7 +953,58 @@ class FieldAttachOtherTestCase extends FieldAttachTestCase {
   +    foreach($entity_types as $entity_type => $entity_create_page) {
   

   missing space after foreach here. (It's a language construct rather than a function, so it follows the standard for control structures.)

5. +++ b/core/modules/field/tests/field.testundefined
   @@ -948,7 +953,58 @@ class FieldAttachOtherTestCase extends FieldAttachTestCase {
   +      $this->assertNoText('Orginial entity missing in hook_field_attach_validate for ' . $entity_type, 'Original entity missing for ' . $entity_type);
   

   The assertion message here should be what is displayed when the assertion is true... so, in this case, it's actually that the original entity is present. (It's when the assertion fails that the original entity is missing.)

6. +++ b/core/modules/field/tests/field_test.moduleundefined
   @@ -261,3 +261,14 @@ function field_test_field_widget_form_alter(&$element, &$form_state, $context) {
   +  drupal_set_message('hook_field_attach_validate runs for ' . $entity_type);
   ...
   +    drupal_set_message('Orginial entity missing in hook_field_attach_validate for ' . $entity_type);
   

   As above, in these messages (and the assertions that test for them), there should be parens on hook_field_attach_validate() to indicate that it's a function.

   Also, we should probably use format_string() to place the $entity_type in the message (both here and in the assertions that test for these messages).

Thanks @adharris. The latest patch looks good to me except for a few minor style issues (and one question):

1. +++ b/core/modules/field/tests/field.testundefined
   @@ -1018,6 +1018,74 @@ class FieldAttachOtherTestCase extends FieldAttachTestCase {
   +class FieldAttachHookTestCase extends FieldAttachTestCase {
   

   We need to add a docblock for the new class, with a summary something like: "Tests field attach hooks for multiple core entity types."

2. +++ b/core/modules/field/tests/field.testundefined
   @@ -1018,6 +1018,74 @@ class FieldAttachOtherTestCase extends FieldAttachTestCase {
   +   * Tests hook_field_attach_validate()
   

   Still missing a period here.

3. +++ b/core/modules/field/tests/field.testundefined
   @@ -1018,6 +1018,74 @@ class FieldAttachOtherTestCase extends FieldAttachTestCase {
   +      $this->assertText(format_string('hook_field_attach_validate() runs for !entity_type', array('!entity_type' => $entity_type)), 'hook_field_attach_validate() runs for ' . $entity_type);
   +      $this->assertNoText(format_string('Orginial entity missing in hook_field_attach_validate() for ', array('!entity_type' => $entity_type)), 'Original entity present in hook_field_attach_validate() for ' . $entity_type);
   

   I was about to say the message texts here should also use format_string(), but, looking at this now, I realize we can probably just leave off the message parameter entirely here. The default message will be informative enough and save us this redundancy. :) (See the assertTextHelper() docs for the message it will display.) What do you think?

4. +++ b/core/modules/field/tests/field_test.moduleundefined
   @@ -261,3 +261,14 @@ function field_test_field_widget_form_alter(&$element, &$form_state, $context) {
   +    drupal_set_message(format_string('Orginial entity missing in hook_field_attach_validate() for ', array('!entity_type' => $entity_type)));
   

   Looks like this message is missing the placeholder. (Oops!)

Leaving at NR for others to weigh in on the question of test placement. (I also probably can't RTBC this since it started out as my patch.)

Actually I realize now my 4th point interferes with the test coverage, so marking NW for that.

Alright, everything in #53 looks correct to me now, and the test-only patch shows the correct assertion failures.

@agentrickard also said in IRC that the patch looks good to him other than that, so let's have someone clean up that minor issue and I think this is RTBC. Thanks!