Skip to content
This repository has been archived by the owner on Jan 31, 2020. It is now read-only.

Validator result interface proposal #24

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

Maks3w
Copy link
Member

@Maks3w Maks3w commented Jul 20, 2015

This is a proposal of ResultInterface about the stateless validator proposal made in:

/cc @bakura10 @zendframework/community-review-team

/**
* Returns the list of error violations in a machine readable format.
*
* @return array
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can constraint this to integer or string types

While this method does not apport new features it improves the expressiveness.

credits @gianarb zendframework#23
public function getTranslationDomain();

/**
* Returns the message without translation and with the placeholders replaced by the variables.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused by this, why "without translation"?
I also - just going by the names of the functions - don't know how I would render the message itself. I assume it'd be

$res = $foo->getValidationResult(); // pseudo

foreach ($res->getMessages() as $msg) {
    echo sprintf("<li>%s</li>", (string) $msg);
}

Alternatively to (string) $msg I'd like to have a clear getter that will give me the error message. Something like $msg->getMessage() or $msg->getText() or $msg->getTranslatedMessage().

All we have in the interface for this is getMessageTemplate() (as far as I can understand it) which in my opinion isn't that clear.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having messages translated means the validator needs to be translation aware, in order to inject each result with a translator instance, so that you can then get the translated messages. That's a lot of pointers running around, for something that is (a) not terribly common, and (b) outside the domain of validation entirely.

It would be simple to create a helper utility that can give back an altered response instance with translated messages:

$results = $validator->validate($data);
if (! $results->isValid()) {
    $results = $utility->translate($results); // Returns a new result instance, with translated messages
}

We already support message templates in a standard format; the main aspect would be getting that and the message variables to inject.

I'm not convinced we need the translation domain, however; that seems like something that the consumer who wants translated messages would provide at the time they request translated messages.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is the ResultInterface defines a model, and models are not responsible how to render itself.

For the above reason couple with a translator is not in my wishlist.

About the need of the translation domain I agree I don't like to have that thing. The point is the result interface does not have any relation with the validator so there is no way of to give a namespace to the translation.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I understand why translation shouldn't happen inside.

However, from a User-PoV the naming then is really confusing. Essentially all I get is a message. I'm able to translate any message by other means, so personally I don't quite see the need to call this TranslatableMessageInterface then. Furthermore the messages would be returned from the validator. So the validator needs to know the textdomain as well so it could pass it into the message object. I feel that this is passing along metadata that may not even be needed.

So This would bring us to what @weierophinney proposed (the way I understand it at least): skip translation.

foreach ($messages as $msg) {
    $translator->translate((string) $msg);
}

Which I feel is perfectly fine. And if you need higher level translation you would probably do something along the lines of

foreach ($messages as $msg) {
    $translator->translate($msg->getMessageTemplate(), $msg->getMessageVariables());
}

From the user point of view (and the ones of people who write docs), i think this is a pretty good syntax. But with this said my question:
Why the need for anything translation related in those messages ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree the name is not the best. I open for ideas.

Do you all agree with constraint __toString to only interpolation? Really I don't care if the message is translated or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you all agree with constraint __toString to only interpolation?

Yes.

@bakura10
Copy link
Contributor

I agree with @manuakasam , this is highly confusing. Usage in my PR is much easier. People should not having to deal with two different objects for retrieving messages error. The workflow should staty like this:

$result = $validator->validate('foo');

if (!$result->isValid()) {
   foreach ($result->getMessages() as $message) {
      echo $message;
   }
}

This is the typical and most common use case. Translation should only be used for people that needs it. I really don't like this proposal :(.

@bakura10
Copy link
Contributor

Also, I'm not sure what you are trying to do here. Trying to integrate the validation result stuff into the 2.x branch? This is a BC, all the validators are not stateless, you are going to go through a lot of pain and hazardous process if you try to make this BC with current architecture.

Also, there are other things I'd like to refactor in the validators themselves to make them more coherent with ZF conventions, so I'd prefer to do all the BC at once.

@bakura10
Copy link
Contributor

Also please have a look at my own implementation for different way to solve the same issue bakura10#1

public function getMessageTemplate();

/**
* Returns the variables to be replaced in the message template placeholders.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variables may should have a cardinal number associated to the value for proper translation of plurals

/cc @DASPRiD

@Maks3w
Copy link
Member Author

Maks3w commented Sep 12, 2015

I like the idea of @weierophinney about to use a helper function for to translate the message

I would like to extend de concept of translate to not only an optional feature for HumanLanguageA <=> HumanLanguageB but make it a requirement for do ErrorCode => HumanLanguageX

In such case the translation tool should look for the error code in a message collection and interpolate the message with the validation context and values cardinality

In resume:

  • Message templates associated to a validator (text-domain)
  • Validation error code for choose a single template (translation template)
  • A validation context collection of values for freedom on make contextualized error messages.
  • Each value have a cardinality associated for proper use of plurals, etc.

@DASPRiD
Copy link
Member

DASPRiD commented Nov 16, 2015

For translations, it could make sense to rely on intl's MessageFormatter, as it works much nicer with formatting multiple different variable types in a single message, as opposed to how gettext's plural handling is always based on a single value.

@weierophinney weierophinney modified the milestones: 3.0.0, 2.6.0 Feb 17, 2016
@weierophinney
Copy link
Member

Marking as 3.0, as returning a result instance is a BC break from current usage.

@snapshotpl snapshotpl mentioned this pull request Jun 27, 2017
@weierophinney
Copy link
Member

This repository has been moved to laminas/laminas-validator. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

  • Squash all commits in your branch (git rebase -i origin/{branch})
  • Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
  • Run the laminas/laminas-migration tool on the code.
  • Clone laminas/laminas-validator to another directory.
  • Copy the files from the second bullet point to the clone of laminas/laminas-validator.
  • In your clone of laminas/laminas-validator, commit the files, push to your fork, and open the new PR.
    We will be providing tooling via laminas/laminas-migration soon to help automate the process.

@michalbundyra
Copy link
Member

This repository has been closed and moved to laminas/laminas-validator; a new issue has been opened at laminas/laminas-validator#49.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants