Hi,

I encountered a small problem with the module in D7:
when I enter or edit a node that uses scheduling, and I only enter a date and leave the (required) time field empty, I get a validation error on saving the node which is OK.

At the same time I also get a system warning (see attached screenshot).

I've taken a quick look at the code and I think you might have to alter the node_save validation a bit to first check all fields before calling _scheduler_strtotime.

Regards and thx for a great module,

eddib.

Comments

wojtha’s picture

Version:7.x-1.0» 7.x-1.x-dev
Status:Active» Needs review
StatusFileSize
new2.35 KB
jonathan1055’s picture

Hi wojtha,
Thanks for taking an interest and making a patch. A little bit of explanation of what you are doing and why would be very helpful. I could work it out from your patch, but it is much better if the original creator explains what they have done, then we can all see why and give a better review ;-)

Jonathan

wojtha’s picture

Well, I though that the patch is expressive enough by itself :-)

I've just added checking if the value is string since in some cases arrays were passed into the _scheduler_strtotime(). I've put these checkings in all places where the _scheduler_strtotime() is being called: node_presave, node_validate and hook_form_alter.

Takki’s picture

I have tested the patch and it works on 7.x-1.0 and 7.x-1.x-dev.

wojtha’s picture

Status:Needs review» Reviewed & tested by the community

based on #4

Eric Schaefer’s picture

I am not able to reproduce the php warnings in your screen shot, even with E_ALL. How did you do it?

jonathan1055’s picture

Status:Reviewed & tested by the community» Active

To Takki in #4, yes the patch applies and gives no error, but we do not know what problem it is addressing.
Also to wojtha in #5, it is bad form to mark your own patch as 'tested and reviewed' in any circumstances. But particularly in this example when you have not demonstrated the initial problem. I'm not saying there is no problem, we just need a clear example of what is going wrong and how to replicate it, then the maintainer can judge if the patch is appropriate. Please do not take offence at my posting, I am not trying to score any point or be defensive, just explaining what we need.
Thanks,
Jonathan

bbdata’s picture

I am able to recreate this error when I use the Date Popup field type in the Scheduler module settings. Then try to edit a node and set only a date and leave the time field empty.

However, my error is slightly different:
Warning: trim() expects parameter 1 to be string, array given in _scheduler_strtotime() (line 466 of sites\all\modules\contrib\scheduler\scheduler.module).

Update:
$node->publish_on is indeed an array and contains a date and time key under the above condition.

tengoku’s picture

In my case, my problem is that when user don't select a date and submit the form (meaning date and time fields in blank) the Warning: trim() expects parameter 1 to be string, array given in _scheduler_strtotime() (line 466 of sites\all\modules\contrib\scheduler\scheduler.module).

thanks of the patch of @wojtha the php error is gone and now it says "A valid date is required for Expires On."

wiifm’s picture

wiifm’s picture

Status:Active» Reviewed & tested by the community
StatusFileSize
new99.89 KB

Here is the problem as best as I can describe it

Create a new content type, this content type has

  • Enable scheduled unpublishing for this content type
  • Require scheduled unpublishing

both ticked in the edit screen for the content type.

  1. Visit node/add/[content_type]
  2. Leave all fields empty on the node add screen
  3. Click "Save"

You will see

Warning: trim() expects parameter 1 to be string, array given in trim() (line 487 of /var/www/[site]/sites/all/modules/contrib/scheduler/scheduler.module).

Backtrace is attached as an image (done with devel with krumo)

Scheduler backtrace

With the patch in #1 this issue goes away. It applies cleanly to 1.x-dev with drush make. So I am happy with it and others seem to be the same as well, so I am re-marking this RTBC unless someone can find a better way to solve this.

rickmanelius’s picture

Status:Reviewed & tested by the community» Needs work

Hi @wiifm. I followed your instructions exactly on 3 different versions of php (5.2.17, 5.3.14, and 5.4.4) with E_All and I'm not able to recreate. Can anyone confirm that this is still an issue with the latest dev branch?

wiifm’s picture

Status:Needs work» Fixed

Cannot reproduce on the latest --dev. Closing for now. Thanks.

jonathan1055’s picture

Status:Fixed» Closed (cannot reproduce)

Thanks for the reply.
Just for the record and for stats, the status should be 'Closed (cannot reproduce)'

However wiifm marked #1546162: trim() expects parameter 1 to be string, array given in _scheduler_strtotime() as a duplicate of this issue, so there could still be a problem. I guess we assume both are fixed until we see more evidence, but there was definitely a problem at some time.

Jonathan

pfrenssen’s picture

Status:Closed (cannot reproduce)» Needs review
StatusFileSize
new1.98 KB

I can reliably reproduce this :) I stumbled on this bug while writing a test for #1069668: Default time with user override. When running the test this error message appears every time when using date popups and introducing a validation error in the date field (for example, only enter the date, leave the time empty). The date_popup module keeps this value as an array until validation passes.

Did not add a specific test for this as this is covered by the test in #1069668: Default time with user override.

jonathan1055’s picture

Status:Needs review» Reviewed & tested by the community

Tested and it works exactly as explained.

It's unfortunate that the inbuilt date popup validation message only says '... does not match the expected format' without actually saying what is wrong and what the expected format is. In our scheduler validation messages we give the expected format layout. But I guess to improve the messaging would require more work in scheduler_node_validate, eg when the value is an array we test other aspects of it, in addition to the date-popup messages. But that would get clumsy. Maybe leave this as is for now. At least your fix removes the system message which was the whole point of this patch.

Thanks. Marking RTBC

Jonathan

rickmanelius’s picture

Status:Reviewed & tested by the community» Fixed

Hi Everyone,
Thanks for all your hard work on this one. Committed!
http://drupalcode.org/project/scheduler.git/commitdiff/a2e9d66?hp=34ea8a...
-Rick

jonathan1055’s picture

Thanks. Just for reference, you marked the patch author as Wojtha, but the original patch in #1 was discarded. Pfrenssen was the author of the actual changes, from #15.

pfrenssen’s picture

Don't worry about it, I'm happy that it got in :)

rickmanelius’s picture

Drat. This was an issue with dreditor showing the patch in #15 when I clicked the 'review' button for each, so I mistakenly thought the patch was the same and thus I gave credit to the initial post. When assigning credit, I'll make sure to load the actual patch rather than rely on dreditor. I was trying to be efficient :)

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

jonathan1055’s picture

Title:Remove system warning when entering invalid date/time» Fix "warning: trim() expects parameter 1 to be string" when entering date with no time

Changing the title to help other users find this issue.

This patch does not need to be ported to D6 as there is no error or warning when entering a popup date and leaving the time empty.