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

Introduce CountryCode validator (rework of #4142) #6888

Closed
wants to merge 7 commits into from

Conversation

adamlundrigan
Copy link
Contributor

This is an update to #4142; I rebased the original branch and updated to include feedback on the initial prototype:

  • Bump copyright year + CS fixes
  • Remove unused resource bundle files
  • Removed hard-coded country names
  • Use static instead of self in Zend\I18n\CountryDb

There is one outstanding issue that I'm unsure of: country codes are not locale-specific, so should this code (or at least the Validator class) be moved out of the I18n namespace? If so, where should the resource class (Zend\I18n\CountryDb) be placed?

Thanks to @mwillbanks for the bulk of the work on this and also to those who left their feedback in the original PR (#4142)

@mwillbanks
Copy link
Contributor

@adamlundrigan thanks for taking care of this, I just haven't had any time to jump in recently.

The I18n namespace was specifically because it dealt with countries, it was more about semantics than it was about what it actually required. Much like the post code validator which lives under the i18n namespace.

I'm still hesitant with the removal of the country names, as that takes away much of the value of this. Generally speaking most people will not utilize it without the list.

@adamlundrigan
Copy link
Contributor Author

@mwillbanks no worries, glad to help.

The I18n namespace was specifically because it dealt with countries, it was more about semantics than it was about what it actually required. Much like the post code validator which lives under the i18n namespace.

Good point...I think my brain was stuck on Locale when that question came to mind

I'm still hesitant with the removal of the country names, as that takes away much of the value of this. Generally speaking most people will not utilize it without the list.

Yeah, I did wonder that myself, but perhaps that can be mitigated in the docs by showing how to get the localized country name themselves using Locale::getDisplayRegion? Or perhaps provide a resource class (or method on CountryDb) to build and return a K=>V list for a given locale on the fly? Either option would eliminate the need for a hard-coded country name list.

@adamlundrigan
Copy link
Contributor Author

On a related note, I stumbled upon HtCountryModule, which does everything this PR does plus add a hydrator strategy. It delegates the handling of the actual country data to phine/lib-country

@mwillbanks
Copy link
Contributor

@adamlundrigan i vote for the phine/lib-country approach! basically having a data loader would work out nicely.

@ojhaujjwal
Copy link
Contributor

Hey guys. I am the author of HtCountryModule. If you guys like the approach of using phine/lib-country, I will be quite happy to create a new PR using phine/lib-country approach.

@mwillbanks
Copy link
Contributor

@ojhaujjwal @adamlundrigan the only core issue with this would be the data being supplied from a 3rd party library / relying on the 3rd party library. I think that would be the only blocker at this point, there might be a data source we could utilize separately but I'll leave that up to you both :)

@ojhaujjwal
Copy link
Contributor

The author of lib-country seems to be quite aware of using a reliable data source. See this issue comment .

@adamlundrigan
Copy link
Contributor Author

The more I think about this the more I feel that perhaps this is one of those cases better handled by a module rather than an addition to the core framework. HtCountryModule is already out there and does the job very well, so why co-opt that functionality?

@weierophinney what do you think?

@glen-84
Copy link
Contributor

glen-84 commented Dec 2, 2014

Why add a dependency on a 3rd-party library when you can just use the built-in Locale class?

This is primarily a validator (according to the title), so country names should not even be a requirement.

If you have to include country names, then I agree with @adamlundrigan WRT a method on the CountryDb class. It should maybe support caching as well though (at least statically).

@weierophinney
Copy link
Member

@adamlundrigan and @mwillbanks — totally agreed; less functionality for us to support in the framework is a good thing, and this is indeed one of the motivators behind a module ecosystem (extending framework functionality).

Closing! Thanks for the work and the discussions!

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.

None yet

6 participants