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.

Sign up for GitHub

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

Jump to bottom

4.x native datetime #12934

Merged
merged 13 commits into from
Feb 12, 2019
Merged

4.x native datetime #12934

merged 13 commits into from
Feb 12, 2019

Conversation

ADmad
Copy link
Member

@ADmad ADmad commented Jan 28, 2019 •
edited
Loading

As discussed in #12820, this PR updates FormHelper to generate native HTML inputs for all available datetime types. I have modified DateTimeWidget accordingly and added new YearWidget to generate selectbox for years.

Still have to do lot of cleanup.

What should be done for the individual FormHelper() methods for generating single selects like day(), hour(), minute(), seconds(), meridian()? My vote is to just drop them. They can still be generated using select() with appropriate options list if some needs them.

@ADmad ADmad added this to the 4.0.0 milestone Jan 28, 2019
@dereuromark
Copy link
Member

dereuromark commented Jan 28, 2019 •
edited
Loading

We should make sure that all the "old" ways have an easy upgrade path to an existing method somewhere.
That eases the migration efforts for existing apps IMO.

best case: in core directly
worst case: e.g. shim plugin helper addition

markstory
markstory reviewed Jan 29, 2019
View reviewed changes
Copy link
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I second moving the existing widget into a plugin for people who want to upgrade but can't update their UI.

src/View/Widget/YearWidget.php Outdated Show resolved Hide resolved
@ADmad
Copy link
Member Author

ADmad commented Jan 29, 2019

Along with existing DateTimeWidget the shim plugin will also need a FormHelper with retains all the options juggling that is currently done for datetime inputs.

@markstory
Copy link
Member

will also need a FormHelper with retains all the options juggling that is currently done for datetime inputs.

Yeah that is going to be awkward 😢

@codecov
Copy link

codecov bot commented Feb 3, 2019 •
edited
Loading

Codecov Report

Merging #12934 into 4.x will increase coverage by 0.11%.
The diff coverage is 94.28%.

Impacted file tree graph

@@             Coverage Diff              @@
##                4.x   #12934      +/-   ##
============================================
+ Coverage     93.14%   93.26%   +0.11%     
- Complexity    13210    13417     +207     
============================================
  Files           470      471       +1     
  Lines         32804    33140     +336     
============================================
+ Hits          30556    30908     +352     
+ Misses         2248     2232      -16
Impacted Files Coverage Δ Complexity Δ
src/Database/Type/DateTimeType.php 95.72% <ø> (ø) 61 <0> (ø) ⬇️
src/View/Helper/FormHelper.php 96.35% <91.66%> (+0.26%) 312 <0> (-49) ⬇️
src/View/Widget/YearWidget.php 92.3% <92.3%> (ø) 10 <10> (?)
src/View/Widget/DateTimeWidget.php 97.43% <96.87%> (-0.65%) 17 <13> (-56)
src/I18n/Middleware/LocaleSelectorMiddleware.php 100% <0%> (ø) 11% <0%> (+5%) ⬆️
src/Routing/Middleware/RoutingMiddleware.php 100% <0%> (ø) 22% <0%> (+7%) ⬆️
src/Http/Middleware/CsrfProtectionMiddleware.php 100% <0%> (ø) 20% <0%> (+3%) ⬆️
src/Http/Runner.php 100% <0%> (ø) 3% <0%> (-2%) ⬇️
src/ORM/Association/DependentDeleteHelper.php 100% <0%> (ø) 4% <0%> (ø) ⬇️
src/ORM/Association/HasMany.php 100% <0%> (ø) 67% <0%> (+15%) ⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f3be66...f5bd76a. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 3, 2019

Codecov Report

Merging #12934 into 4.x will decrease coverage by 0.06%.
The diff coverage is 94.93%.

Impacted file tree graph

@@             Coverage Diff              @@
##                4.x   #12934      +/-   ##
============================================
- Coverage     93.15%   93.09%   -0.07%     
+ Complexity    13202    13130      -72     
============================================
  Files           468      469       +1     
  Lines         32801    32625     -176     
============================================
- Hits          30556    30371     -185     
- Misses         2245     2254       +9
Impacted Files Coverage Δ Complexity Δ
src/View/Helper/FormHelper.php 94.8% <93.33%> (-1.29%) 333 <0> (-28)
src/View/Widget/YearWidget.php 95% <95%> (ø) 14 <14> (?)
src/View/Widget/DateTimeWidget.php 97.05% <95.83%> (-1.02%) 15 <11> (-58)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 840bbcc...135800e. Read the comment docs.

@ADmad ADmad force-pushed the 4.x-native-datetime branch from 135800e to f9c1faf Compare February 4, 2019 18:35
@ADmad ADmad changed the title WIP - 4.x native datetime 4.x native datetime Feb 4, 2019
@ADmad
Copy link
Member Author

ADmad commented Feb 4, 2019

This is ready for review.

dereuromark
dereuromark reviewed Feb 4, 2019
View reviewed changes
src/Database/Type/DateTimeType.php
@@ -50,6 +50,7 @@ class DateTimeType extends BaseType
*/
protected $_format = [
'Y-m-d H:i:s',
'Y-m-d\TH:i:s',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we backport this to 3.x?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, it would be useful to those already using datetime-local input.

@dereuromark
Copy link
Member

Will you open a 4.x branch on Shim plugin to proof the upgrade possibilities for the former inputs?
Or how do we proceed here to not forget to enable the migration path here?

@ADmad
Copy link
Member Author

ADmad commented Feb 5, 2019

Yeah I'll make a PR to your Shim plugin.

The more taxing job is updating the docs :)

@ADmad ADmad mentioned this pull request Feb 8, 2019
Merged
@dereuromark
Copy link
Member

dereuromark commented Feb 8, 2019 •
edited
Loading

All right, we have a branch now: https://github.com/dereuromark/cakephp-shim/tree/4.x#new-shims
Test passing.

ADmad added a commit to cakephp/docs that referenced this pull request Feb 9, 2019
@ADmad
Add info regarding native datetime inputs.
aa9906f 
Refs cakephp/cakephp#12934
@ADmad ADmad mentioned this pull request Feb 9, 2019
Merged
@ADmad
Copy link
Member Author

ADmad commented Feb 9, 2019

Ancillary work of PR to Shim plugin and docs PR is done. This should be good to merge now.

@markstory markstory self-assigned this Feb 11, 2019
@markstory markstory merged commit 27519de into 4.x Feb 12, 2019
@markstory markstory deleted the 4.x-native-datetime branch February 12, 2019 04:26
This was referenced Feb 12, 2019
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dereuromark dereuromark dereuromark left review comments

@markstory markstory markstory left review comments

Assignees

@markstory markstory

Labels
None yet
Projects
None yet
Milestone
4.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@ADmad @dereuromark @markstory