Convert tokens to Drupal 8

Forgot to remove the comment reminder about the issue.

Thanks for looking at this. How did you test the changes? I know that the token API is now in core, and that a browsable list of tokens is provided by contrib token module. However, that still does not create a filter option to enable when editting the text input filters (like we did in 7.x). Hence there are no calls to our hook_tokens() and the text just gets displayed as-is in the node body. Does 8.x provide a token filter for use when editing nodes? I guess I must be mising something ... any help appreciated.

When I see how they work I can start to write tests, as requested by Pieter in the issue summary.

I had trouble testing the tokens as I couldn't use them in text input filters as you mentioned, and the 'node' group of tokens didn't show in the 'Browse available tokens' popup in most places. However I did find that the 'node' group of tokens, that the scheduler tokens belong to, are shown when using the pathauto module. If you create a pattern in the pathauto module you'll see the scheduler tokens appear and you can test them by setting the tokens in the pattern and then creating a node. It's a bit fiddly and I'm hoping there's a better way such as a token filter or something, but it allowed me to fix the hook_tokens().

I’ve tried to write a test class for this functionality. It’s my first test I’ve written so I look forward to any feedback.

I tried setting the publish on and unpublish on variables in the $settings array when the node is created but it wouldn’t work so I ended up setting the values after node creation.

Thanks Joe, this is useful. Just to let you know, because the current commited code does not pass all tests yet, the testing of any new patches initiallly shows 'waiting for branch to pass'. To test your patch it needs to be kicked-off manually by clicking on the grey 'waiting to pass' link, then clicking the button 'Do not wait for branch to pass'. I have just done this, so we should see some results.

Thanks - I didn't know you could do that.

So it looks as though we don't get any more failures, but I'm not sure why we get two extra passes, I thought it would only be one? Unless the setUp() counts as one?

Also, the testUserPermissions that is currently failing - would that be fixed by #2651448? If so maybe we could commit the patch in that issue as a short term fix, as it does solve the problem, it just might not be the most elegant way of solving it.

Hi Joe,

the testUserPermissions that is currently failing - would that be fixed by #2651448?

Actually, all of the current test failures have patches in the issues for review which fix every failing test. Take a look at the summary of #2594615: Automated testing in 8.x [meta] for links the issues. The only reason I have not committed them yet is that I want Pfrenssen to review them. It's not wise for one maintainer to commit their own patches without some kind of review. If you would like to look at those patches, please do. I have just commented on #2651448-16: Publish and Unpublish fields are shown for users who do not have the permission

Regarding the new test, yes setup() gives an extra class pass (which is a pain). I have a few suggestions for an improved patch, which I will post here soon.

When adding a new test class, it should always be tested separately, without the accompanying code fixes. This will show if the tests are actually covering the faults being corrected. Here is the just the new test from #7

Wrong patch, here is just the new test.

Ok so it looks like the code does fix the failure in the test. Are there any other improvements for the test?

To answer your questions from #7

I tried setting the publish on and unpublish on variables in the $settings array when the node is created but it wouldn’t work so I ended up setting the values after node creation.

To set the scheduler dates when using DrupalCreateNode you just need to pass the raw unix timestamp number as the field value. When editing a node using DrupalPostForm you need to pass the formatted date and time strings to the separate fields.

Ok so it looks like the code does fix the failure in the test

Yes it does, that's good.

Are there any other improvements for the test?

File .tokens.inc is fine, but I have re-rolled the patch with the following changes to the test file:

1. Removed public static $modules = ['filter', 'node', 'scheduler'];. The 'node' and 'scheduler' modules are already loaded via schedulerTestBase and 'filter' is not actually required in the tests.
2. I used the unix timestamp values to set the scheduler dates during node creation as explained above.
3. Instead of just testing the 'raw' format, I made an array loop to test the default, long, raw and custom formats.
4. I have added a debug output row, but will remove this before commiting.

Great work on writing your first test, thanks. I used all of your work but just added a few improvements. If you like what I've done, mark it RTBC and I'll commit it.

Setting back to 'needs review'. Soon we will not have to do this each time, as when the committed codebase passes all the tests the status will remain at 'needs review' providing the patch still passes.

When editing a node using DrupalPostForm you need to pass the formatted date and time strings to the separate fields.

That's why it wasn't working - I was trying to do this but I wasn't using DrupalPostForm.

Thanks for your feedback. The changes you've made look good to me :)

Thanks for your work on this.

Ignore the failed patches above. They were queued but not run until the 8.x committed codebase passed all tests. That happened with my commit a few minutes ago, and then the untested patches have suddenly come to life and been run. This issue is already fixed.

Welcome back Pieter. No apology needed.
We have made some progress :-)

Wow you've been busy! Welcome back and good luck with your new business :)

Yeah I think we've made some good progress, really close to an alpha release now!