Rename tel into phone to ensure compatibility with Symfony 3.4 form_div_layout

[JusteLeblanc]

I updated the library to make it compatible with last form_div_layout of symfony 3.4

To fix the issue https://github.com/misd-service-development/phone-number-bundle/issues/83

[nealio82]

I suspect the wrong line was changed?

leaving ln 120 as $view->vars['type'] = 'tel'; but changing ln 191 to return 'phone'; fixes the exception for me

EDIT: oh, i just looked at the rest of the PR with all the other changes done for completeness. literally just changing ln 191 seems to make everything work in easyadminbundle!

[teohhanhui]

Why not just rename it to misd_phone_number?

[giovkanata]

@rh389 @Collaborators Can we merge it?

[JusteLeblanc]

Hello @rh389 I need help on this PR:

1. Tests are red as they are not passing anymore on PHP HHVM, should we stop to ensure compatibility with PHP HHVM ?
2. I renamed tel_widget into phone_widget to ensure compatibility with Symfony 3.4+ form_div_layout. Other ideas suggested by reviewers: custom_tel_widget or misd_phone_number_widget. My favorite one is custom_tel_widget. Any preference? cc @teohhanhui @nealio82 @toooni

Thanks!

[teohhanhui]

I really think it should use proper namespacing, so misd_phone_number

[robhogan]

This is a tricky one - in principle I like namespacing but I think there's a decent argument for phone_number_widget, for consistency with phone_number_helper, phone_number_format and phone_number_of_type.

Is there any reason to namespace one when the others are not?

[teohhanhui]

If it's a Twig filter, I could understand why it's not namespaced, as it'd be too verbose to write in templates. But otherwise, we should namespace everywhere.

[JusteLeblanc]

This is a tricky one - in principle I like namespacing but I think there's a decent argument for phone_number_widget, for consistency with phone_number_helper, phone_number_format and phone_number_of_type.

Is there any reason to namespace one when the others are not?

I agree with phone_number_widget for consistency reason. cc @teohhanhui I updated the PR.

@rh389 could we merge it as tests are red because of PHP5.3 and PHP HHVM builds? Other builds are 👍

[JusteLeblanc]

@rh389 Any feed-back on this PR ? Would be great to merge it :)

[robhogan]

Looks good, just haven’t had time try it out and to update the docs - if you’re happy to update the readme that’d be great, otherwise I’ll merge when I get chance. Cheers for the work👍🏻

[JusteLeblanc]

Looks good, just haven’t had time try it out and to update the docs - if you’re happy to update the readme that’d be great, otherwise I’ll merge when I get chance. Cheers for the work👍🏻

I've updated Changelog. I've found nothing to update in the Readme. I tried out the bundle, it works fine. 😸 I removed PHP 5.3 on which build failed as it's not maintained since August 2014. 🙈

All tests are green except PHP hhvm ones. I will have a look on it.

@rh389 Sounds good for you? 😇

[JusteLeblanc]

I specified Symfony version for PHP hhvm build as Symfony 4 ended HHVM support. All tests are green! ✅

@rh389 It's ready to be merged 😸

[JusteLeblanc]

I specified Symfony version for PHP hhvm build as Symfony 4 ended HHVM support. All tests are green! ✅

@rh389 It's ready to be merged 😸

@rh389 is there anything I could do to help you merge this PR?

[robhogan]

Many thanks for your work and patience @JusteLeblanc - merged!