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

Validator message format #1051

Merged
merged 5 commits into from
Oct 23, 2013
Merged

Conversation

cebe
Copy link
Member

@cebe cebe commented Oct 23, 2013

Fixes #991.
ICU message format is now available for gridview summary and all validator messages.
It is not available for client validation as some params are added in JS.

@samdark
Copy link
Member

samdark commented Oct 23, 2013

Have you meant ICU instead of ITU?

@cebe
Copy link
Member Author

cebe commented Oct 23, 2013

Have you meant ICU instead of ITU?

Yeah, of course :)

they are not responsible for i18n ;)
@samdark
Copy link
Member

samdark commented Oct 23, 2013

It seeems there's a lot of duplication between format and translate. I guess format can be used inside translate.

@cebe
Copy link
Member Author

cebe commented Oct 23, 2013

It is quite duplicated but the error messages are not that nice anymore when we just use format in translate.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 3631890 on cebe:validator-message-format into 752037d on yiisoft:master.

qiangxue added a commit that referenced this pull request Oct 23, 2013
@qiangxue qiangxue merged commit 9f7ca5d into yiisoft:master Oct 23, 2013
@qiangxue
Copy link
Member

Let me merge it first. Good job!

@qiangxue
Copy link
Member

I refactored the code as @samdark suggested. The large chunk of duplicated code is not good. For this reason, we can tolerate the less perfect error message.

I also think perhaps we should reorganize/redesign the current MessageFormatter and FallbackMessageFormatter:

  • Instead of extending from the intl MessageFormatter, we create a new MessageFormatter class which mainly provides the method format($message, $params, $language)
  • The format() method will use intl MessageFormatter to do the actual work if intl extension is installed. Otherwise, it will call its fallback implementation.
  • I1n8 class can implement getMessageFormatter() and drop format().

What do you think?

@cebe
Copy link
Member Author

cebe commented Oct 24, 2013

Why? Imo having I18N::translate() and I18N::format() looks like a good interface to me. You hardly ever need the message formatter instance. Doing $i18n->getMessageFormatter($language, $pattern)->format($args); does not look that good to me.

@samdark
Copy link
Member

samdark commented Oct 24, 2013

@qiangxue what's the con to have MessageFormatter extended as it is now?

@qiangxue
Copy link
Member

Ok, we can keep I18N::format().

The current MessageFormatter is fine if we plan to let it be used alone (but then you won't be able to use FallbackMessageFormatter). But since we modified it with replaceNamedArguments(), I suspect anyone will use it alone.

The design of FallbackMessageFormatter is very clever. However, it is a bit twisted, and it may also cause problem to phar support like this: #261.

@cebe
Copy link
Member Author

cebe commented Oct 24, 2013

You can use the MessageFormatter or FallbackMessageformatter alone if you want. There is just no sense in using the fallback one as Messageformatter already checks for intl extension installed.

whats the problem with phar?

@qiangxue
Copy link
Member

This is what I proposed: https://gist.github.com/qiangxue/6641b25650d8699952ba

@samdark
Copy link
Member

samdark commented Oct 24, 2013

@qiangxue why extends Component? It's not using anything from component, isn't it?

@cebe
Copy link
Member Author

cebe commented Oct 24, 2013

Extending Component is fine as it is a yii class. idea looks okay to me. fallback handling is not correct though.

@samdark
Copy link
Member

samdark commented Oct 24, 2013

@cebe but it's not used anywhere: there are no events, behaviors, getters or setters. Also it's not used as a component.

@qiangxue
Copy link
Member

This is just a 5-minute work to explain the idea. The class can extend from Object instead of Component.

@cebe
Copy link
Member Author

cebe commented Oct 24, 2013

I think provided idea is better than current solution. Will handle it together with #1062.

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

Successfully merging this pull request may close these issues.

Rethink validator and BaseListView summary parameter replacement
4 participants