Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Form][FrameworkBundle][Bridge] Add a DateInterval form type#16809
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
MisatoTremor commentedMar 11, 2016
For the upcoming hack day and if you find the time to review this for any issues I should fix, a short notice about that would be nice. |
| <tagname="form.type" /> | ||
| </service> | ||
| <serviceid="form.type.dateinterval"class="Symfony\Component\Form\Extension\Core\Type\DateIntervalType"> | ||
| <tagname="form.type"alias="dateinterval" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thealias attribute is now not needed anymore.
95037ca to3586785CompareMisatoTremor commentedMar 13, 2016
Type alias removed and rebased. @xabbuh Sorry for being late... |
fabpot commentedJun 16, 2016
👍 |
| <serviceid="form.type.datetime"class="Symfony\Component\Form\Extension\Core\Type\DateTimeType"> | ||
| <tagname="form.type" /> | ||
| </service> | ||
| <serviceid="form.type.dateinterval"class="Symfony\Component\Form\Extension\Core\Type\DateIntervalType"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A service for this type is not needed as it has no dependency, we just use FQCN since 2.8. Btw almost all the others have been deprecated in 3.1.
HeahDude commentedJun 17, 2016
Apart some very minor comments, this PR looks awesome! |
3586785 to77eff03CompareMisatoTremor commentedJun 17, 2016
@HeahDude Thanks for your comments. Some of these stem from the fact that this PR was originally based on 2.7. I have updated it accordingly for 3.1+ now. The in-linings would break the soft limit, so I left them out. |
fabpot commentedJun 17, 2016
Can you inline even if it breaks the soft limit? We are not following any limits for line length. |
77eff03 to39ac6deCompareMisatoTremor commentedJun 17, 2016
@fabpot OK, done :) |
| private $fields; | ||
| /** | ||
| * Constructor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should be removed
MisatoTremor commentedJun 18, 2016
@xabbuh I did this in sync with the existing classes, i.e. |
fabpot commentedJun 21, 2016
@MisatoTremor Let's do the change for this one for now. |
39ac6de to2f8f79dCompareMisatoTremor commentedJun 21, 2016
@fabpot Done :) |
| throw new TransformationFailedException(sprintf('The fields "%s" should not be empty', implode('", "', $emptyFields))); | ||
| } | ||
| if (isset($value['invert']) && !is_bool($value['invert'])) { | ||
| throw new UnexpectedTypeException($value['invert'], 'boolean'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The resulting error message won't help the end user as 'invert' won't be anywhere, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't quite understand what you mean with "won't be anywhere".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The error message is going to beExpected argument of type "boolean", xxx" given. I don't think there will be anything in the context mentioning that the error comes from theinvert value and not the whole value for the form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You are right, I oversaw this when applying HeahDudes comments. Fixed
2f8f79d to43ad054CompareAlso add dateinterval widget to twig templates.
43ad054 tof7669beComparefabpot commentedJun 22, 2016
Thank you@MisatoTremor. |
…m type (MisatoTremor)This PR was merged into the 3.2-dev branch.Discussion----------[Form][FrameworkBundle][Bridge] Add a DateInterval form type| Q | A| ------------- | ---| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#13389| License | MIT| Doc PR |symfony/symfony-docs#4817Replaces#15030Commits-------f7669be [Form] Add a DateInterval form type Also add dateinterval widget to twig templates.
| public function __construct(array $fields = null, $pad = false) | ||
| { | ||
| if (null === $fields) { | ||
| $fields = array('years', 'months', 'days', 'hours', 'minutes', 'seconds', 'invert'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would use an associative array here instead of plain array (because you often usearray_flip, andisset($array[$key]) is faster thanin_array($key, $array))
MisatoTremor commentedJun 22, 2016
Thanks all for your help improving this and thanks for merging. |
| $fields = array('years', 'months', 'days', 'hours', 'minutes', 'seconds', 'invert'); | ||
| } | ||
| $this->fields = $fields; | ||
| $this->pad = (bool) $pad; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@MisatoTremor We're missing the actualpad property at the top of this class
This PR was merged into the 3.2-dev branch.Discussion----------[Form][#16809] add missing pad property| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#16809 (comment)| License | MIT| Doc PR |Commits-------647e6ba [#16809] add missing pad property
Replaces#15030