[RFC] Forms and Service layer

[adamlundrigan]

I've combined proposed changes to the forms and service layer into one as the latter depends heavily on the former. This one actually is an RFC as I haven't implemented a prototype yet.

Service Layer

We can simplify the service layer by pushing concerns such as the object being hydrated/extracted and password hashing on save -- both of which the service currently handles -- out of the service layer.

• CreateUserForm can instantiate an empty instance of the user class itself
• Password hashing can be performed by the hydrator when required

Q: Putting the password hasing in the hydrator might be an ill-conceived idea. Any better ideas that avoid having the hashing hard-coded into the service? I just don't like it in the service...it should be part of the form handling process. A filter, maybe?

interface UserServiceInterface
{
    /**
     * @return UserEntityInterface
     * @throws RuntimeException on failure to create user
     */
    public function create(array $data);

    /**
     * @return UserEntityInterface
     * @throws RuntimeException on failure to update user
     */
    public function update(UserEntityInterface $user, array $data);

    /**
     * @return UserEntityInterface
     * @throws RuntimeException on failure to delete user
     */
    public function delete(UserEntityInterface $user);

    // Convenience; proxy directly to mapper methods
    public function findById($value);
    public function findByUsername($value);
    public function findByEmail($value);
}

Q: Proxy find* methods through to mapper, or just provide a getter for the mapper as the current service does? My preference is on proxying the methods so we're not forcing userland code to reach through the service to get at the mapper to run a query.

A class implementing this interface would require three dependencies

• Mapper instance
• CreateUserForm instance
• UpdateUserForm instance

NOTE: CreateUserForm and UpdateUserForm are instances of the same class (UserForm). They are created separately by the FormElementManager so that modules extending ZfcUser could attach custom fields differentially based on the action occurring.

Form Layer

The login form and input filter would remain largely unchanged except for some updates now that we're away from PHP 5.3.x. The registration form and input filter will evolve to work for both create and update operations:

UserForm (formerly BaseForm)

In terms of elements it will be unchanged.

Q: Should we drop the CAPTCHA? IMO it shouldn't be baked in as not every application needs/wants it. Consuming app can always pull the form and inject the CAPTCHA if they need it.

UserInputFilter (formerly RegisterFilter)

In terms of elements it will be unchanged.

The RecordExists and NoRecordExists validators should be updated to use the context argument so that they work for both create and update operations.

Q: Do we need both? RecordExists isn't actually used by ZfcUser.

Q: Should the validators be changed to allow calling arbitrary mapper methods, like the new authentication layer does?

[claytondaley]

Zend/Form already has intelligence to handle passwords so this concern is resolve by PR #563, possibly eliminating the need to make yet another BC change.

[adamlundrigan]

@claytondaley good point, the new_password stuff isn't really necessary then. Document updated

[claytondaley]

I also really like the idea of implementing password hashing through a Hydrator Strategy. In fact, it would be great if the Hydrator was implemented to be easily extensible.

• For example, I have PR that converts ZfcUserAdmin's EditForm to use a hydrator.
• Now I need to extend it to properly Hydrate DateTimes.

It'd really be better if ZfcUserAdmin used (extended) a core hydrator... and I could extend it further. Unfortunately, I'm still on the learning curve for Hydrator Strategies so I'd benefit from a recommended pattern for this. My go-to was the event system, but my research on performance suggests this may not be the best long-term strategy.

I assume a hydrator strategy also eliminates any hard-coding since you could replace the default right there in the hydrator.

[adamlundrigan]

I also really like the idea of implementing password hashing through a Hydrator Strategy. In fact, it would be great if the Hydrator was implemented to be easily extensible.

I've been battling with that myself, as it's not the purpose of the hydrator. The hydrator is meant to have one responsibility: converting between an object and it's array representation. The proper solution would be to do this on the form post-validation but pre-object-populate, so I'm thinking the best course of action would be to create a filter which hashes the value of the password field.

For example, I have PR that converts ZfcUserAdmin's EditForm to use a hydrator. Now I need to extend it to properly Hydrate DateTimes.

I had worked on something similar last summer but never got the chance to complete it and PR it (I did recently get as far as opening a ticket for it: Danielss89/ZfcUserAdmin#65). A lot of what ZfcUserAdmin does should be part of ZfcUser, and that module should just be the UI and integration with ZfcAdmin; the logic of CRUDing users is something ZfcUser should handle itself...and that's the main goal of my recent PR blitz here, which when complete I had planned to turn my attention to ZfcUserAdmin next.

[claytondaley]

You're right. This is really what InputFilters do. The Zend\Form Quick Start gives examples like:

             'filters' => array(
                 array('name' => 'Zend\Filter\StringTrim'),
                 array('name' => 'Zend\Filter\StringToLower'),
             ),

If an InputFilter can trim and change case, why not hash? If the stored hash uses a different strategy, you can always getRawValues() and rehash.

[claytondaley]

... and I finally figured out the clean way to handle hydrator extension.

• In the Factory (updated to be consistent with Go-forward Form extension strategy? #576, sample implementation here),
  • Set the stock hydrator and filter (may depend on options or the serviceLocator)
• In init() (called by FormElementManager, see below)
  • Add the stock form elements
  • Apply any stock hydrator strategies and filters
• In the extending module,
  • Use a Delegator to extend the Form (updated to be consistent with Go-forward Form extension strategy? #576, sample implementation here)
  • In callback, add elements
  • In the same callback (right after you add the element), use $form->getHydrator() to access the hydrator and add the related strategy (if needed).

[claytondaley]

For the ZfcUser forms, I'd also like to recommend a dynamic validation group implementation comparable to Rob Allen's recommendation. I care more about the requirement than the strategy -- add/remove methods for the validation group would be just as good (and perhaps better).

This is particularly important on "edit" forms where you may want to present readonly data that should not be committed back to the database. The current method for achieving this is to create a validation group. Unfortunately, the default validation group implementation depends on explicit knowledge of all of the elements on the form, which is antithetical to easy extension.

I believe the readonly attribute should also exclude by default, with the option to override by setting exclude => false. IMHO the core Zend/Form should be upgraded in this way, but this is as good a place to start as any.

[claytondaley]

A final enhancement would be a universal form renderer placed in a parent project (ZfcMvc?) that is inherited by ZfcUser and ZfcAdmin. ZfcUserAdmin has the right idea importing _form.phtml into numerous pages. I believe I've successfully implemented a fully generic example that would be a drop-in replacement for existing ZfcUser forms. Several notes:

• This strategy still hard-codes a DOM structure (in this case dl and dt). I assume it's OK (even desirable) in Zfc* to hard code a standard so it's easier to write a universal CSS skin. As part of a BC break, however, we might consider an upgrade (ZfcUserAdmin#63 offers an alternative).
• At the same time, I dislike the way the DOM structure is hard-coded. For example, nested collections won't benefit from the current approach. It would be nice to figure out how to do this at more of a configurations level so that we need only call $form->form().
• I also think we should inject the redirect element into the form in the controller (hidden type, excluded from validation) then let the rendering engine actually add it.

[adamlundrigan]

RE: validation groups this is something I've brought up in discussion about ZF3's forms, as the current implementation of validation groups makes it hard to work with complex forms. IMO there should be a convenience wrapper around that so you can selectively include and exclude fields without having to specify the entire validation group. In one of my projects last year I wrote an atrociously dirty hack to work around the issue, and it's on my someday agenda to extract and clean up that code so it can be released.

RE: form rendering, I think that's best left up to the consuming application. We'll bundle a minimum viable set of view scripts (like we do now) but if those don't fit your use case you'll need to override them. The approach ZfcUser currently uses (looping over every element in the form) is likely the behavior that will remain, though more than likely anyone adding custom fields would also be overriding the view scripts to control how they are displayed. ZfcUserAdmin's handling of this annoys me a bit as it includes the common form template rather than sending it through the view renderer, making it unnecessarily difficult to override.

RE: redirects, I'm not sure about this one. IMO there are better ways to handle the post-login redirect than sticking the target in a hidden form element. That's prone to abuse if not done properly, as we saw last year with the open redirect security issue. It's something I plan to give some thought to once the current round of PRs I've opened are wrapped up.

[claytondaley]

Obviously, there's no code to react to so the current state is the available point of reference

• Redirect is important for an authentication module.
  • The current approach would be significantly improved by adding the element to the form around the time it's created. It would automatically populate when setData() is called and be automatically returned with an invalid form. There seems no advantage to the current approach.
  • I'm also skeptical that you can do much more than plug bugs in redirect logic. A user provides the first URL by typing it into the browser and you make sure -- at the end of the process using a 302 -- that the user's browser is the one that calls that URL again. I don't see how a modified form submission can be any worse than the original URL -- both provided by the user.

---

There are several points in your response on forms. Trying to separate them out:

• I believe the register.phtml loop would work perfectly for login.phtml as well. Thus, ZfcUser 2.x has two blocks of code when one would suffice. The ZfcUserAdmin strategy is DRY by sharing that code between both pages.
• Granted, the ZfcUserAdmin strategy would be even better if it you could configure that script from the Controller... but the current state is no worse than ZfcUser's equally hard-coded current state.
• Finally, the generic loop I developed takes the loop-replaces-login idea to the logical conclusion. Minus a (now-fixed) bug for button labels, it should exactly reproduce ZfcUser and ZfcUserAdmin forms
  • To make sure I didn't stick my foot in my mouth, I went ahead and replaced every ZfcUser 1.x form with the generic renderer (I'm using ZfcUserAdmin so I have to stay on 1.x). There are some quirks... for example, the ChangeEmail form doesn't add() a submit button in __construct. Besides that, the forms are as expected -- materially indistinguishable.

[claytondaley]

Ran into another quirk worth adding to the punchlist. Custom Form Elements only work when you use two specific strategies:

• The form must be created using the FormElementManager (e.g. $sl->get('FormElementManager')->get('\Application\Form\MyForm');)
• The elements must be added during init() rather than __construct().

To address an issue, I also (re)discovered:

• The Form must be listed in the form_elements section rather than the service_manager section to ensure that init() is called (see Documentation around FormElementManager->get() zendframework/zendframework#7305 for related discussion)