Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NFC] Add EntityFormTrait to pricefieldForm - standardised getEntityId() #17516

Merged
merged 1 commit into from
Jun 16, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

This is an alternative to https://github.com/civicrm/civicrm-core/pull/17514/files

Really we want to use a standardised way to get the entity id from an edit form
& _fid is kinda wierd

Before

No clear way to retrieve the entity id from PriceFieldForm

After

It uses the EntityFormTrait & like other forms that do the public function getEntityId() is availablle.

Technical Details

Other tidy up changes - use getEntityId() internally on the form. Add functions required to support inclusion of entity form trait, don't pass default parameters to CRM_Utils_Request::retrieve.

Comments

Note that this does not convert fully to the entity form which would reduce code, declare fields by metadata, support defaults in url & support custom field on the entity

This is an alternative to https://github.com/civicrm/civicrm-core/pull/17514/files

Really we want to use a standardised way to get the entity id from an edit form
& _fid is kinda wierd
@civibot
Copy link

civibot bot commented Jun 6, 2020

(Standard links)

@mlutfy
Copy link
Member

mlutfy commented Jun 6, 2020

I definitely like the cleaner code and potential for eventual cleanup :)

Question: In order to save the Entity ID for the postProcess hook, do we also have to implement/override setEntityId() and call it after the create?

Test fails seem unrelated:

    CRM_Core_CommunityMessagesTest.testGetDocument_NewOK_UpdateFailure_CacheOK_UpdateOK with data set #0
    CRM_Core_CommunityMessagesTest.testGetDocument_NewOK_UpdateFailure_CacheOK_UpdateOK with data set #1
    CRM_Core_CommunityMessagesTest.testGetDocument_NewOK_UpdateFailure_CacheOK_UpdateOK with data set #2
    CRM_Core_CommunityMessagesTest.testGetDocument_NewOK_CacheOK_UpdateOK
    CRM_Queue_QueueTest.testTimeoutRelease with data set #0

@mlutfy
Copy link
Member

mlutfy commented Jun 6, 2020

jenkins, test this please

@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton
Copy link
Contributor Author

@mlutfy it would be nice to get rid of $_fid in favour of $_id but at this stage it should work because the preProcess does

    $this->_fid = CRM_Utils_Request::retrieve('fid', 'Positive', $this)

Note that calling retrieve like that also does

      $store->set($name, $value);
  • effectively $form->set('_fid', $value);

On submit it passes through that line again & if it is no longer in the url then it gets it from the $form->get

So it should all work without an extra setEntityId()

@eileenmcnaughton
Copy link
Contributor Author

test this please

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@mlutfy
Copy link
Member

mlutfy commented Jun 16, 2020

@eileenmcnaughton It didn't seem to work for me in a postProcess hook, i.e. the "id" was not available in the params passed to the hook (when adding new price fields, it was OK when editing an existing PF).

I had to add this at the end of postProcess:

     if (!is_a($priceField, 'CRM_Core_Error')) {
+      $this->setEntityId($priceField->id);
       CRM_Core_Session::setStatus(ts('Price Field \'%1\' has been saved.', [1 => $priceField->label]), ts('Saved'), 'success');
     }

although happy to merge this, and continue the discussion separately, since it's a good NFC cleanup.

@eileenmcnaughton
Copy link
Contributor Author

@mlutfy ok - let's merge & check - that makes sense

@mlutfy mlutfy merged commit 9130c7c into civicrm:master Jun 16, 2020
@mlutfy mlutfy changed the title Add EntityFormTrait to pricefieldForm - stdised getEntityId() [NFC] Add EntityFormTrait to pricefieldForm - standardised getEntityId() Jun 16, 2020
@eileenmcnaughton eileenmcnaughton deleted the pf_field branch June 16, 2020 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants