zendframework / zend-inputfilter

InputFilter component from Zend Framework
BSD 3-Clause "New" or "Revised" License
64 stars 50 forks source link

fixxed #94 'Missing required failure message for CollectionInputFilter' issue #170

Closed ruzann closed 5 years ago

ruzann commented 6 years ago

Added default message to display when a collection is required. Added mechanism for setting a custom "required" message. Updated factorie to allow setting that message.

ruzann commented 6 years ago

@froschdesign Is there any updates relation to pull request?

froschdesign commented 6 years ago

@ruzann I still see a problem here, because your changes introduce a hard coded error message to an input-filter, that means we have message constants in input-filters and validators. This ends in another problem, because the error message can not be translated.

froschdesign commented 5 years ago

@ruzann Do not create a new translator object, this will introduce new problems and you must set the the translation files for the input-filter - this is wrong! Please revert this and use the translator from the validator.

ruzann commented 5 years ago

@froschdesign can you please review?

ruzann commented 5 years ago

@froschdesign all your notes fixed, please check again, when you have a time.

newdevonair commented 5 years ago

@froschdesign is there any news from this pull request? I need this functionality in my project too.

Thanks.

froschdesign commented 5 years ago

@ruzann Some pseudo code:

function getNotEmptyValidator()
{
    if ($this->notEmptyValidator === null) {
        $this->notEmptyValidator = new NotEmpty();
    }

    return $this->notEmptyValidator;
}

function setNotEmptyValidator($notEmptyValidator);

function prepareRequiredValidationFailureMessage();

The default error message already exists and you can replace the validator with your own. messageRequired and the related methods are not needed.

(Yes, we do not use the validator for validation, but we allow a translation of the error message with the standard way.)

What do you think?

/cc @malukenho @rkeet

@newdevonair You can also add a review or a comment to the current suggestions. 😉


Sorry for the late response and the short review. My focus is currently on the documentation, website and zend-navigation.

rkeet commented 5 years ago

I would suggest same as @froschdesign review: own getter/setter for storing NotEmpty instance.

Otherwise seems alright to me, apart from tests not passing ;)

ruzann commented 5 years ago

@froschdesign I fixed your notes, please check again.

tigran-m-dev commented 5 years ago

Hi guys, do you have any updates related this issue? Is it ready to merge or have any other improvement? If you need some help I am ready to help.

froschdesign commented 5 years ago

@weierophinney

Alternately, we should allow specifying a validator and/or validator chain at the top level for checking the status of the collection, prior to attempting to validate the members of the collection.

This one! (See my example with pseudo code above.) Why? In all inputs we uses the messages from the validator. Setting a message to an input would introduce a new behaviour for this component. Setting a validator or a validator chain is not new. The validator has already the option to translate a message.

ruzann commented 5 years ago

@weierophinney, @froschdesign is there any news? should I change something?

weierophinney commented 5 years ago

@ruzann Please see the feedback above - @froschdesign and I feel that the approach outlined here is convoluted, and inconsistent with the architecture. Adding a validator chain specific to the collection input filter as an entity would make more sense. If you can refactor to that approach, we can review again.

ruzann commented 5 years ago

@froschdesign is there any news?

froschdesign commented 5 years ago

@ruzann Your improvement will be included in the next minor release (2.9). At the moment I am working at documentation of zend-hydrator, therefore I can not give a time frame. I'm sorry.

In the meantime, just use your own fork.

weierophinney commented 5 years ago

Thanks, @ruzann!