Add CRM_Core_Form::isFormInViewMode and CRM_Core_Form::isFormInEditMode

[mlutfy]

Overview

Extensions implementing the buildForm hook often have to make sure that $form->elementExists() before applying a certain logic, otherwise QuickForm will throw a fatal error if the field is missing.

Sometimes we use it as a lazy way of checking the mode of the form, which we can get through $form->getMode() and then try to remember which of the CRM_Core_Action constants we have to check against.

I stumbled on CRM_Core_Form::isFormInViewOrEditMode and found it rather practical, but it is set as protected, so we cannot call it from a buildForm hook, which has $form, not $this.

Comments

The function was added in 2018-01 as part of 240b0e6, which is fitting. I stumbled on a similar bug in an extension just now, while trying to delete a profile field.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[mattwire]

@mlutfy Not blocking and happy to merge. But does it make sense to have isInViewMode() and isInEditMode() instead?

[mlutfy]

@mattwire Maybe it's lazyness on my part, but I mostly need a way to avoid running buildForm code during a deletion, and that function seems to work for most use-cases.

(Form View is still a normal form, just usually frozen, and BROWSE/BASIC/ADVANCED/PREVIEW are rarely used).

[eileenmcnaughton]

@mlutfy can we please do what @mattwire suggested

[mlutfy]

OK, pushed an update :)

[eileenmcnaughton]

As an aside the 'ready for review' tag should probably be added by someone other than the PR submitter. The fact the PR exists & doesn't have [WIP] in the header means the submitter thinks it's ready. The tag should indicate someone else does as well

[mlutfy]

Makes sense - although it's not easy to see PRs where feedback was given, and the author responded. Then again, most authors can't add tags, so it makes sense to keep this tag for reviewers (and not self-tag).

[mattwire]

it's not easy to see PRs where feedback was given, and the author responded

@mlutfy That's what I created the tag for and intended it be used for. I add it on my own PRs and others in exactly that situation.

[demeritcowboy]

Is the function contents mismatch going to be confusing? I would expect from the wording that if someone were to refactor

function isFormInViewOrEditMode() {
  return $form->isFormInViewMode() || $form->isFormInEditMode();
}

that it would be identical, but that wouldn't always be true here. Is there a reason for leaving out BASIC etc? You said they're not often used, but are they never used when this is called?

[eileenmcnaughton]

Yeah - the thing about 'ready to review' is that it can't be added by un-permissioned users - whereas pings & 'request review ' can & it implies that these ones are ready & others are not.

I don't personally factor it's existence or not into whether I look at a PR - since I haven't found PRs tagged with it as any more ready-to-review than others , but some people may do.

[eileenmcnaughton]

Perhaps we should automatically tag ALL PRs as 'ready for review' & then we can REMOVE the tag if it turns out not to be? I think I might find it useful if that were the case

[eileenmcnaughton]

@mlutfy I've removed the MOP because there is feedback by @demeritcowboy - can you check that out respond to it.

[eileenmcnaughton]

@mlutfy also - please self-merge this if you can agree it with @demeritcowboy - I don't think we need further involvement by others as whatever you & @demeritcowboy agree seems fine in this context

[mlutfy]

@demeritcowboy Good point - I think those actions were added for "action links", and most of them are basically a type of "Edit" mode. I updated my patch.

[demeritcowboy]

Thanks looks good. As per earlier comments "merge on pass".

[mlutfy]

Thanks! It's a really minor detail, but I often get bit by this in extensions. It'll make things cleaner!