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.Hello, i get 2 notice, when visiting nodes, after enabling and configuring this module and tokens.
Notice: Undefined property: stdClass::$unpublish_on in scheduler_tokens() (line 47 of \sites\all\modules\contrib\scheduler\scheduler.tokens.inc).
Notice: Undefined property: stdClass::$publish_on in scheduler_tokens() (line 47 of \sites\all\modules\contrib\scheduler\scheduler.tokens.inc).
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | 2934328-6.tokens_property_and_test.patch | 3.12 KB | jonathan1055 |
Comments
Comment #2
PushaMD CreditAttribution: PushaMD as a volunteer and commentedmini-patch, checking if the node has the property.
Comment #3
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedHi PushaMD,
Thanks for reporting this. Yes I can replicate this error and your patch does solve it. I have checked and in 8.x we don't get this warning.
Jonathan
Comment #4
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedActually, I thought it would be good to cover the case of 'no dates' in the automated testing. Here's a patch which adds to the existing
testTokenReplacement()but without the fix in #2 so we should get warnings about the undefined properties.Comment #6
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThat's good, as expected. Now adding the fix from #2 with the test change.
Comment #7
nipany CreditAttribution: nipany commentedSome minor changes on fix from #2
Comment #9
nipany CreditAttribution: nipany commentedUpdated patch on fix from #2
Comment #10
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedHi nipane,
Thanks for your interest in this issue. However, your patches do not seem quite right, they revert code in the earlier patches, and also do not include the test coverage that was added in patch #6. In these issue threads with patches we always want to build on the latest existing patch so could I request the following:
(a) please explain what you think is wrong with the patch in #6
(b) make your changes on top of the changes in #6 so that the new test coverage is inculded in you patch
(c) if possible provide an interdiff text file if the changes you are making are not obvious, so that other can review you patch more easily
Thanks
Jonathan
Comment #11
ciss CreditAttribution: ciss at yousign GmbH commentedTLDR: Current patch is in #6.
The suggested change in #7 and #9 doesn't make (any obvious) sense. I'm hiding those patches in order to shift focus back to the patch in #6.
Comment #12
smartsystems160 CreditAttribution: smartsystems160 commentedpatch #2 returns error: patch unexpectedly ends in the middle of line
patch -p0 --dry-run < checking_property-2934328-1.patchpatch #6 returns error: patch unexpectedly ends in the middle of line. Hunk #1 FAILED at 44.
patch -p0 --dry-run < 2934328-6.tokens_property_and_test.patchComment #13
hitfactory CreditAttribution: hitfactory commentedAttaching re-roll of #6 so it applies to latest 7.x-1.x.
Comment #14
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThanks for re-rolling. However, there wasn't actually anything wrong with the patch in #6 and your re-roll in #13 is actually identical, apart from the fact that you have swapped round the order of the two files being changed (scheduler.tokens.inc followed by scheduler.test). I have just re-tested #6 and there are no warnings in the jenkins console log when applying the patch.
Comment #16
hitfactory CreditAttribution: hitfactory commentedI had blindly followed the comment in #12 thinking that was the reason the patch had not been accepted, but now I see it's fine.
I can confirm that the Undefined property etc notices disappear with the patch. However, I can't vouch for the updates to the tests as I haven't looked at them too deeply.
Comment #17
glynster CreditAttribution: glynster commented+1 RTBC as this did not seem to reach the last dev release. Any reason why? I was flooded again with notices are updating to the latest version. Applied this patch and they were all removed.
Comment #18
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedHi glynster,
No reason other than it was not actually marked RTBC, so I have not committed it yet. Looks good, so will do soon.
Comment #20
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedCommitted and fixed.
Thanks PushaMD for the original patch.
Comment #21
glynster CreditAttribution: glynster commented@jonathan1055 thanks so much!