Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
- Go to admin/config/content/scheduler
- Set the following options:
- Date format: "d-m-Y"
- Field type: "Standard text field"
- Date only: not checked
These settings are accepted when the form is submitted, but it should actually fail, since if no time part is supplied in the date format, the user should enable the 'date only' option and provide a default time, so Scheduler knows at which time the content should be scheduled.
Comment | File | Size | Author |
---|---|---|---|
#14 | 2143061_13_date_format_without_time-test_only.patch | 1.94 KB | pfrenssen |
#13 | 2143061_13_date_format_without_time.patch | 6.59 KB | jonathan1055 |
Comments
Comment #1
pfrenssenComment #3
pfrenssenComment #4
jonathan1055 CreditAttribution: jonathan1055 commentedIt is not obvious to me that this should be an error. Before we had the default time functionality a format such as d-m-Y was acceptable. The user entered the date, and there was an implicit default time of 00:00 on that day. It still works ok like that now.
If the admin wants to specify a time other than 00:00 then they can, but they should not be forced to tick the option. I suppose using the date-only option does give the benefit that the description displays the default time, but that is the only difference as far as user experience is concerned.
Was there a particular test scenario which caused you to want this, or was it just a separate idea?
Jonathan
Comment #5
pfrenssenI thought it was a bug, but apparently it is a feature :)
Comment #6
jonathan1055 CreditAttribution: jonathan1055 commentedI'd be ok if you wanted to implement this, you didn't need to close it yet ;-) The change does have the advantage that the users then see the time that will be used.
I did not actually look at your patch, but I have done now and at first glance you've done quite a few useful changes. Marking as 'needs review' and I will look at in detail tomorrow.
Jonathan
Comment #7
jonathan1055 CreditAttribution: jonathan1055 commentedHaving considered the coding in the patch, I think that this is worth doing. The benefit is that the user is told the default time in the text beneath the entry box. Also you have re-factored scheduler_admin_submit() to use internal functions, and removed the call to the date_popup function. The messages are better formed too.
The patch does not apply to the current git .module or .test, due to the recent patches committed, but I tested the code manually and it works as expected. Just one point - coder review does not like the call to
drupal_set_message(implode(' ', $messages))
Hopefully you can find a neat way to avoid that, as it is really helpful now that coder review gives a clean pass of .module.
Jonathan
Comment #8
jonathan1055 CreditAttribution: jonathan1055 commentedMaking the title into a positive statement
Comment #9
jonathan1055 CreditAttribution: jonathan1055 commentedHow about the simple, but clear (and coder-review clean):
Comment #10
jonathan1055 CreditAttribution: jonathan1055 commentedThe patch needed re-rolling as a lot of code has been changed since Nov 2013.
In .module I kept it the same except (a) $format was no longer used and the two lines had been merged into one when setting $time_format, so I replicated that, and (b) I incorporated my drupal_set_message suggestion from #9 to keep it coder-compliant.
For .test the change to $this->drupalCreateUser is not needed as we have already added the 'administer scheduler' permission since the original patch.
So, essentially the same patch. If you are OK with this it can be committed.
Jonathan
Comment #12
jonathan1055 CreditAttribution: jonathan1055 commentedI have re-worked the existing tests into the new loop, first for textfield, then for date_popup. This is better (and hence safer) than directly setting the config variables, as we have found out before. Patch is against 1.2+11-dev
Comment #13
jonathan1055 CreditAttribution: jonathan1055 commentedPatch needed re-rolling, now against 7.x-1.2+15
Comment #14
pfrenssenI already forgot about this one :) At my previous project this functionality was needed, they are already using this in production for many months.
Very good point to save the settings through the interface rather than manipulating the variables. This is a more true functional test. This looks pretty good to me.
Uploading the test only, this should fail.
Comment #16
pfrenssenTest only patch failed, perfect, good to go!
Comment #17
pfrenssen