[Form] Add tel and color types #22679

apetitpa · 2017-05-09T14:12:19Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#22596
License	MIT
Doc PR	not provided yet

javiereguiluz · 2017-05-09T14:15:14Z

CONTRIBUTORS.md

@@ -182,9 +182,8 @@ Symfony is the result of the work of many people who made the code better
 - Larry Garfield (crell)
 - Julien Falque (julienfalque)
 - Martin Schuhfuß (usefulthink)
- - apetitpa
+ - Arnaud Petitpas (apetitpa)


You can revert this change because that file is autogenerated, so any change will be lost.

I was not aware of this sorry, I'll do it in a minute thank you

jvasseur · 2017-05-09T14:33:13Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/form.xml

+            <deprecated>The "%service_id%" service is deprecated since Symfony 3.1 and will be removed in 4.0.</deprecated>
+        </service>
+        <service id="form.type.color" class="Symfony\Component\Form\Extension\Core\Type\ColorType" public="true">
+            <deprecated>The "%service_id%" service is deprecated since Symfony 3.1 and will be removed in 4.0.</deprecated>


This sounds wrong, a new service shouldn't be already deprecated.

Indeed, I reverted this part thank you

ro0NL · 2017-05-09T14:46:58Z

src/Symfony/Component/Form/Extension/Core/Type/TelType.php

+
+use Symfony\Component\Form\AbstractType;
+
+class TelType extends AbstractType


i definitely prefer TelephoneType here 🤔

I was quite unsure about what name to use, so I chose the one that matches the "tel" HTML5 input name but I have to admit it is less clear

i think fully qualified is preferred :)

I'll rename the class then thank you !

@javiereguiluz agree?

Still no consensus about how the class should be named ? @fabpot @nicolas-grekas @xabbuh

Hello, I'd like to finish this PR, how should I name the class ? TelType, TelephoneType, PhoneType etc ? @symfony/deciders @fabpot

either way no blocker for me :) i understand we just want type="tel" one way or another 👍 (my reasoning still stands though(.

I think we should go with types that match the html types because that's we we tend to do instead of trying to name them based of the semantic of the field (CheckboxType, TextareaType, RangeType are good examples of this).

@ro0NL I think your reasoning was wrong here when stating this (about "tel"):

we should not name our class after an implementation detail

HTML5 is exactly the opposite to an implementation detail: it's a public interface, a standard, the tip of the iceberg. So we should definitely not seek to be more clever than the ppl who wrote it (we're not ;) ).

issei-m · 2017-05-09T16:35:23Z

Looks good😀
btw would "TelephoneType" be better to be named "TelType" in accordance with type-name of html5 tag? (I'm not familiar with English, so not sure the abbr like "tel" is common used though.)

issei-m · 2017-05-09T16:48:15Z

would "TelephoneType" be better to be named "TelType" in accordance with type-name of html5 tag?

Oops, I've just missed the review-comment by @ro0NL. Never mind this comment.

ro0NL · 2017-05-09T16:50:01Z

my reasoning; tel === telephone, so TelephoneType is explicit/verbose. In my domain i'd also call this field $user->telephone, not $user->tel.

ogizanagi · 2017-05-09T17:51:11Z

src/Symfony/Component/Form/Extension/Core/Type/ColorType.php

+     */
+    public function getParent()
+    {
+        return __NAMESPACE__.'\TextType';


TextType::class ? (there is no reason not to use ::class now that PHP >= 5.5.9 is required)

I agree but I would have to replace it in other classes too then ?

You should only do this on new classes. We usually avoid doing changes on existing code when it doesn't fix anything nor doesn't really improve the situation, in order to avoid useless conflicts when the maintainers are merging lower branches to upper ones and interfering in git history.

@ogizanagi Thank you for sharing this piece of information, I'll make the change !

ogizanagi · 2017-09-21T20:22:58Z

Needs rebase to fix the conflict. (Let's keep TelephoneType btw, in case there is still debate)

apetitpa · 2017-10-01T22:45:33Z

@ogizanagi @fabpot rebased with 3.4 (sorry for the delay)

fabpot · 2017-10-01T23:17:35Z

@apetitpa Can you have a look at the tests, they seem to be broken by your changes.

apetitpa · 2017-10-02T09:09:50Z

@fabpot It's done, it's green again. Travis 7.1 failing test will be fixed when merging.

nicolas-grekas

with minor comment

nicolas-grekas · 2017-10-02T10:52:38Z

src/Symfony/Bundle/FrameworkBundle/Resources/views/Form/color_widget.html.php

+<?php
+
+/*
+ * This file is part of the Symfony package.


no other file in this directory has the license header, we should remove them

It was suggested by fabbot, but I'll revert the changes ok

apetitpa · 2017-10-02T12:15:44Z

@nicolas-grekas It's done but I still have one concern regarding the name of the TelephoneType class. Should we keep this name or use the original suggested name which was TelType as the HTML input type

nicolas-grekas · 2017-10-02T12:20:31Z

If HTML5 is fine with "tel", so am I.

apetitpa · 2017-10-02T12:45:48Z

@nicolas-grekas Done, reverted to TelType as agreed

nicolas-grekas · 2017-10-02T13:00:38Z

Thank you @apetitpa.

carsonbot added Status: Needs Review Form Feature labels May 9, 2017

javiereguiluz added this to the 3.4 milestone May 9, 2017

javiereguiluz reviewed May 9, 2017

View reviewed changes

jvasseur reviewed May 9, 2017

View reviewed changes

ro0NL reviewed May 9, 2017

View reviewed changes

ro0NL approved these changes May 9, 2017

View reviewed changes

ogizanagi reviewed May 9, 2017

View reviewed changes

lyrixx changed the title ~~[Form] Add tel and color types~~ [Form] Add telephone and color types May 11, 2017

nicolas-grekas changed the base branch from master to 3.4 May 23, 2017 16:54

ogizanagi approved these changes Sep 21, 2017

View reviewed changes

ogizanagi added Status: Reviewed and removed Status: Needs Review labels Sep 21, 2017

fabpot approved these changes Oct 1, 2017

View reviewed changes

apetitpa added 8 commits October 2, 2017 10:00

add tel and color types

9b7c39d

remove duplication and add full name

6a08ffb

add tests for new types

60917c3

add boostrap 3 layout tests for new types

bb0f316

revert contributors to master branch

9980a84

revert form.xml to master branch

4c431e6

update templates with new types and rename TelType to TelephoneType

b6d7177

fix block prefix

081e64e

apetitpa added 2 commits October 2, 2017 10:00

update minimum version required

3c14d32

use class reference

3c5f3eb

nicolas-grekas approved these changes Oct 2, 2017

View reviewed changes

apetitpa changed the title ~~[Form] Add telephone and color types~~ [Form] Add tel and color types Oct 2, 2017

rename and fix tests

e26e0ca

nicolas-grekas closed this in 579e3b3 Oct 2, 2017

apetitpa deleted the add-form-types branch October 2, 2017 22:21

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form] Add tel and color types #22679

[Form] Add tel and color types #22679

apetitpa commented May 9, 2017 •

edited by ogizanagi

Loading

javiereguiluz May 9, 2017

apetitpa May 9, 2017

jvasseur May 9, 2017

apetitpa May 9, 2017

ro0NL May 9, 2017

apetitpa May 9, 2017 •

edited

Loading

ro0NL May 9, 2017

apetitpa May 9, 2017

ro0NL May 9, 2017

apetitpa May 16, 2017

apetitpa Sep 21, 2017 •

edited

Loading

ro0NL Sep 21, 2017

jvasseur Sep 21, 2017 •

edited

Loading

nicolas-grekas Oct 2, 2017 •

edited

Loading

issei-m commented May 9, 2017

issei-m commented May 9, 2017

ro0NL commented May 9, 2017

ogizanagi May 9, 2017 •

edited

Loading

apetitpa May 9, 2017 •

edited

Loading

ogizanagi May 9, 2017 •

edited

Loading

apetitpa May 9, 2017 •

edited

Loading

ogizanagi commented Sep 21, 2017 •

edited

Loading

apetitpa commented Oct 1, 2017

fabpot commented Oct 1, 2017

apetitpa commented Oct 2, 2017 •

edited

Loading

nicolas-grekas left a comment

nicolas-grekas Oct 2, 2017 •

edited

Loading

apetitpa Oct 2, 2017

apetitpa commented Oct 2, 2017 •

edited

Loading

nicolas-grekas commented Oct 2, 2017

apetitpa commented Oct 2, 2017

nicolas-grekas commented Oct 2, 2017


		use Symfony\Component\Form\AbstractType;

		class TelType extends AbstractType

[Form] Add tel and color types #22679

[Form] Add tel and color types #22679

Conversation

apetitpa commented May 9, 2017 • edited by ogizanagi Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apetitpa May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apetitpa Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvasseur Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

issei-m commented May 9, 2017

issei-m commented May 9, 2017

ro0NL commented May 9, 2017

ogizanagi May 9, 2017 • edited Loading

Choose a reason for hiding this comment

apetitpa May 9, 2017 • edited Loading

Choose a reason for hiding this comment

ogizanagi May 9, 2017 • edited Loading

Choose a reason for hiding this comment

apetitpa May 9, 2017 • edited Loading

Choose a reason for hiding this comment

ogizanagi commented Sep 21, 2017 • edited Loading

apetitpa commented Oct 1, 2017

fabpot commented Oct 1, 2017

apetitpa commented Oct 2, 2017 • edited Loading

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apetitpa commented Oct 2, 2017 • edited Loading

nicolas-grekas commented Oct 2, 2017

apetitpa commented Oct 2, 2017

nicolas-grekas commented Oct 2, 2017

apetitpa commented May 9, 2017 •

edited by ogizanagi

Loading

apetitpa May 9, 2017 •

edited

Loading

apetitpa Sep 21, 2017 •

edited

Loading

jvasseur Sep 21, 2017 •

edited

Loading

nicolas-grekas Oct 2, 2017 •

edited

Loading

ogizanagi May 9, 2017 •

edited

Loading

apetitpa May 9, 2017 •

edited

Loading

ogizanagi May 9, 2017 •

edited

Loading

apetitpa May 9, 2017 •

edited

Loading

ogizanagi commented Sep 21, 2017 •

edited

Loading

apetitpa commented Oct 2, 2017 •

edited

Loading

nicolas-grekas Oct 2, 2017 •

edited

Loading

apetitpa commented Oct 2, 2017 •

edited

Loading