Port file_example module to Drupal 8

I'm on it :)

Great! Thanks. :-)

Also a friendly reminder that we have the examples module checklist: #2209627: [meta] Module Checklist for Examples

@Mile23 thanks for the link!!

@fran seva -- howdy! Do you mind if I grab this one?

@Torenware I also can help you.

@Andrew.Mikhailov: that'd be great.

To make things a bit easier, I've set up a github repo at https://github.com/torenware/drupal8-examples. I'll be working off the file-example-workspace

At this point, I've set up the basic structure. I have:

• The routing, permissions, and info YAML files.
• A stubbed .module file
• Stubbed classes for a controller, and the main form.
• The simpletest class, which actually runs with lots of failures :-)

The goal now is to get more and more of this implemented until the tests run green. TDD for the win!

A good thing to start with is to get the menu links set up correctly so the File Example appears in the Tools menu. This will be like the other example modules that are already done.

If you're more ambitious, then you can start working on the form class. The goal is to make it as close as possible to the original D7 code.

I'll start out working on getting the stream class working. Once that's up, I'll start looking at the form itself.

Just clone the repo, and get yourself set up.

Quick progress report: I now have a bit over 1/2 of the tests working. The stuff in file.inc hasn't changed much at all, but the way D8 handles URLs broke a surprising lot of stuff.

One thing that works but is deprecated. The stream wrapper demo puts a file system over part of the $_SESSION global. This is a no-no; see Access session data through the Request object. But making the stream wrapper work using the recommended API is turning out to be pretty hard, and isn't really the point of the demo anyway. So I'm open to suggestions as to how best handle that.

I've also run into trouble trying to do something that should be easy: create a link to a non-managed file. D8 really does not want to do that. Anybody know how to work around that? This is used in the demo so you can see the contents of any files created by the demo.

I'll start looking over the portions of the tests that are failing in a bit, but the demo is already pretty usable.

First patch. This could be "needs work" (it does), but it needs some other eyes on this at this point.

Working at this point:

• The form class. When used interactively, everything I try seems to work.
• The session-based streamwrapper class: works at least as well as the D7 version. Some issues I found in the D7 class have been fixed; the SimpleTest class didn't hit them, and I only found the issues interactively
• The ported test class mostly works, and I get "645 passes, 22 fails, 21 exceptions, 307 debug messages". More on the fails below.

Not working:

• The menu links and the tool menu. Not hard, just haven't gotten to them yet. Andrew is free to do them if I don't get to them.
• The session code is the same as D7, which works, but isn't "kosher/hallal" -- there's a Core Fatwa on this issue. Not sure how to be fully in compliance with that Change Record. Any ideas?
• While a couple of the test FAILs have to do with links that D8 does not allow, the rest seem to be problems where I catch the exception generated in my code. but SimpleTest seems to grab the exception before my catch block ever sees it. This is very strange, but since the locations in the SimpleText logs are in those parts of the code, I'm not sure what else could be going on. If someone will run the tests themselves to see if they have more insight into this than I do, much obliged.
• the URL link generator behind the Url utility class is fragile, and throws exceptions lustily on non-managed files. There's an issue in Core for this #2539622: File uri field type has default formatter uri_link, throws exception when trying to render, and Bendir gave a suggestion for a work-around on Drupal Answers. But the URL generator is pretty unstable right now. If folks are cool with the "this Link" links going to a managed public file instead, I can ignore the issue and just finish the example.

+++ b/file_example/file_example.info.yml
@@ -0,0 +1,7 @@
\ No newline at end of file

I'll get the extra/lack space records on the next patch. Mostly, I'm interested in feedback on the "not working" issues.

the URL link generator behind the Url utility class is fragile, and throws exceptions lustily on non-managed files

Seems I've fixed it. Also I replace deprecated functions

The menu links and the tool menu. Not hard, just haven't gotten to them yet. Andrew is free to do them if I don't get to them.

Added only menu I've added file *.links.menu.yml, I'm investigating how to add menu to toolbar.

The ported test class mostly works, and I get "645 passes, 22 fails, 21 exceptions, 307 debug messages". More on the fails below.

I got on my local machine next result http://joxi.ru/n2Yn0Qdu3DR1A6
657 passes, 16 fails, 55 exceptions, 309 debug messages

I'm down to 0 fails 0 exceptions on the test, so I have a fully working version.

I will look at Andrew's version in a bit. Getting the tests to pass was rather hard; I needed timplunkett help to figure out the URL issues, which were surprisingly nasty. Nastier than just about anything else :-)

I'll also supply an interdiff, since this is going to be fairly large. As before, the github link is a good quick way to see what progress looks like.

I'm going to post my patch for as-of-now state. I've loaded both my version and Andrew's to the github repo, mainly because the two versions are different enough that a merge is going to take a while :-/

If someone can suggest a good graphical diffing tool, much obliged!

If someone can suggest a good graphical diffing tool, much obliged!

if you use PHPstorm, you can select 2 files and press ctrl/cmd+D
Also you can use Meld http://meldmerge.org/
Best regards.

OK, first pass on the merge did not go so well.

The difference between the two versions are of several kinds:

• I've fixed quite a lot of issues that were causing fails or exceptions; simpletest passes w/o problems. My first pass on the merge introduced 4 fails, and a mess of exceptions.
• To get some of the tests to work, Drupal 8 needs download URLs to be granted explicit permission via hook_file_download.
• There were a lot of minor cosmetic issues. I use drupalcs to scan files and to enforce Drupal standards. Those changes were very minor, but there were quite a few of them.
• Andrew introduced newer APIs over some of the older deprecated ones. This is the right thing to do. If we can figure out why the merged version isn't testing clean, it will help. Not all of the changes are completely equivalent, it turns out.

I"m done for today, and probably for most of the week. Andrew may want to compare the versions and see if he can come up with a merge that passes the tests.

I've got a merged version of the example up on github.

I got in Andrew's use of the modern APIs, and the menu link. A minor exceptions to that:

+++ b/file_example/src/Form/FileExampleReadWriteForm.php	(revision )
@@ -0,0 +1,666 @@
+      $wrapper = \Drupal::service('stream_wrapper_manager')->getViaUri($filename);
+      $url = self::getExternalUrl($wrapper);
+      $_SESSION['file_example_default_file'] = $filename;

Not sure how this works. It might not, since getExternalUrl works a little different in my version, and the URL related code in the stream wrapper class was changed to make it more like the stream wrapper classes in Core. In general, the URL generator is a dangerous piece of code in D8.

I'll post the full patch and an interdiff soon.

I've added my latest patch, and an interdiff between Andrew's patch and my latest.

Quick notes:

• getMimeType() is no longer in Drupal's stream wrapper interface, so I removed it from the port.
• I eliminated accessSession() from the controller, since I ended up using Core's approach for accessing files stored in our demo stream wrapper class. Core uses \Drupal\system\FileDownloadController::download() as the controller class to do this for private and temporary files, and I now do the same.
• Generally, I found that modeling the example's stream wrapper around Core's implementation for the temporary: scheme to be the approach that works best.
• The merged code runs all tests w/o error.
• I've run drupalcs to get the code as style compliant as I could. The remaining "errors" are not removable, since \Drupal\Core\StreamWrapper\StreamWrapperInterface itself is not compliant (since it's based off a non-Drupal interface).

What needs to be done before we can commit this and close the issue out:

• Somebody other than Andrew.Mikhailov or I needs to look the code over. In particular, are we explaining the right things? Also, I'm not sure if the doc comment formats are what's required.
• We're still using pre-Drupal 8 style $_SESSION manipulation. This is deprecated, although I'm not getting the change record's substitute version to work for me.
• The URL code is more complex than I want it to be, mostly because getting the Url to work w/o throwing an exception is a chore. @timplunkett agrees that the URL generator class is bad, bad dog, so if we're lucky, this will get changed in Core and we can simplify the code here.

This is annoying.

The testbot installs to a subdirectory of DOCUMENT_ROOT. The fail seems to be due to some of my generated links not resolving due to this problem.

The SIMPLETEST-ME button works fine when I run the tests, so it's gotta be related to running from a subdirectory.

This will need fixing, if only because the URL code in the example needs to be robust against this kind of install.

This is a fun one. We run perfectly under the GUI for SimpleTest. But @timplunkett's suggestion, I tried run-test.sh. and wouldn't you know, it fails under the script. Which the testbot uses.

Not sure how that differs, but at least there's something to debug.

I'm out of ideas on the test failure. I've looked it as an issue on the test bot (#2598720: Fails on testbot, works locally under subdir drupal using run-test.sh). Hopefully somebody can shed some light on this.

Thanks for sticking with it.

OK, so we've got some failures but they seem to be related to just one thing, and might be solvable by the testbot. It might also be that as we move forward with other things we'll find a solution.

So here's a sort of first-phase review of some obvious stuff.

1. +++ b/file_example/file_example.module
   @@ -0,0 +1,65 @@
   + * The File Example module is a part of the Examples for Developers Project
   + * and provides various Drupal File API Examples. You can download and
   + * experiment with this code at the
   + * @link http://drupal.org/project/examples Examples for Developers project page. @endlink
   + *
   + * See @link http://drupal.org/node/555118 Drupal File API @endlink for handbook
   + * documentation on the File API and
   + * @link file File summary on api.drupal.org @endlink for the function summary.
   

   We really want to describe what the module does, not where to find the code.

   We also need to describe, somewhere, the difference between managed and unmanaged files.

2. +++ b/file_example/file_example.module
   @@ -0,0 +1,65 @@
   +/**
   + * Control access to private file downloads and specify HTTP headers.
   

   This should say 'Implements hook_file_download().' https://www.drupal.org/coding-standards/docs#hookimpl

   We'll fudge the coding standards a bit and leave the rest of the docblock.

3. +++ b/file_example/file_example.module
   @@ -0,0 +1,65 @@
   + * @see file_download()
   

   Needs @see hook_file_download() as well.

4. +++ b/file_example/file_example.module
   @@ -0,0 +1,65 @@
   +  // Check to see if this is a config download.
   

   Explain what a 'config download' is, since it's kind of the thing we're demonstrating. :-)

5. +++ b/file_example/file_example.permissions.yml
   @@ -0,0 +1,13 @@
   +'read private files':
   +  title:  See private files in the demo.
   +'read temporary files':
   +  title:  See temporary files in the demo.
   +'read session files':
   +  title:  See session files in the demo.
   

   'See [whatever] files in the File Example module' (Like the first permission in the file)

6. +++ b/file_example/file_example.routing.yml
   @@ -0,0 +1,27 @@
   +file_example.description:
   ...
   +filed_example.fileapi:
   

   It'd be great if we had the description content at the top of the fileapi form, and just got rid of the controller.

7. +++ b/file_example/file_example.services.yml
   @@ -0,0 +1,10 @@
   +# To implement a stream wrapper, we need to register it with the system.  We can either do this
   +# manually by calling up the 'stream_wrapper.manager' service, or, as we do here, have
   +# the system autoload it by tagging the service.
   

   Explain what's happening, then explain the other option. So something like:

   "We create a stream wrapper service here using tags. We could also manually add our service to 'stream_wrapper.manager'."

8. +++ b/file_example/src/Controller/FileExampleController.php
   @@ -0,0 +1,30 @@
   +      '#markup' => $this->t('The file example module provides a form and code to demonstrate the Drupal 7 file api. Experiment with the form, and then look at the submit handlers in the code to understand the file api.'),
   

   'Drupal 7.'

   Also, like I mentioned before, fold this content into the form and remove the controller.

9. +++ b/file_example/src/Form/FileExampleReadWriteForm.php
   @@ -0,0 +1,661 @@
   +   * @todo Remove direct manipulation of the session.
   +   */
   +  public function buildForm(array $form, FormStateInterface $form_state) {
   +    if (empty($_SESSION['file_example_default_file'])) {
   +      $_SESSION['file_example_default_file'] = 'session://drupal.txt';
   +    }
   +    $default_file = $_SESSION['file_example_default_file'];
   +    if (empty($_SESSION['file_example_default_directory'])) {
   +      $_SESSION['file_example_default_directory'] = 'session://directory1';
   +    }
   +    $default_directory = $_SESSION['file_example_default_directory'];
   

   Hell no. :-)

   We have 'session' service. Inject that if we really need the session.

   Also, I think what's happening here is that we're using a session variable to pass the 'default' around. This can be replaced with a config or state. And will be. :-)

10. +++ b/file_example/src/Form/FileExampleReadWriteForm.php
    @@ -0,0 +1,661 @@
    +    }
    ...
    +      $_SESSION['file_example_default_file'] = $file_object->getFileUri();
    ...
    +  private static function getExternalUrl($file_object) {
    

    Again, inject that service.

11. +++ b/file_example/src/Form/FileExampleReadWriteForm.php
    @@ -0,0 +1,661 @@
    +  /**
    +   * Helper function to get us an external URL if this is legal, and to catch
    +   * the exception Drupal throws if this is not possible.
    +   */
    +  private static function getExternalUrl($file_object) {
    

    I'm not sure if this is necessary or factored properly, but leave it for now.

12. +++ b/file_example/src/Form/FileExampleReadWriteForm.php
    @@ -0,0 +1,661 @@
    +    try {
    +      // If the Uri is unroutable (such as for a temporary file), or if Drupal cannot create
    +      // a link, we will throw here:
    +      if (is_string($url)) {
    

    Wrap at 80 chars.

13. +++ b/file_example/src/Form/FileExampleReadWriteForm.php
    @@ -0,0 +1,661 @@
    +      $_SESSION['file_example_default_file'] = $filename;
    

    DIE. :-)

14. +++ b/file_example/src/Form/FileExampleReadWriteForm.php
    @@ -0,0 +1,661 @@
    +      $destination = \Drupal::service('file_system')->tempnam('public://', 'file');
    

    Inject this service.

15. +++ b/file_example/src/Form/FileExampleReadWriteForm.php
    @@ -0,0 +1,661 @@
    +    $_SESSION['file_example_default_file'] = $destination;
    

    Kill with fire.

@Mile23 --

Much thanks. I'll look these over this evening.

Yeah, settings is the right thing to do for some of this.

For the rest of the session stuff, yeah, I'd love to use the session service. I tried this and had to back it out; the D7 code uses references to $_SESSION subarrays, which was easy to move around. The session service can only read an attribute and write an attribute, so I'm trying to figure out how wrap the session object in such a way that I can keep a pointer into the session. Having the tests work helps a lot for taking this on, since when I bailed on this, I was pretty far from a clean test.

@Mile23 --

Got a start at this. The following is done and pushed to my github staging area:

• I'm using DI for the State API, FileSystem, and ModuleHandler objects.
• I've switched every reference to $_SESSION in the form class to use the State API, except for the one reference that clears our "file system", since the code in the stream wrapper class still uses sessions.

One thought; we likely put the file system over $_SESSION in D7 because it was the simplest thing you could do that would make a reasonable stream wrapper demo. Given the changes in the API for sessions in D8, it's actually the same amount of work to use the State API as it would be to manipulate the session. Doing a "State Space" stream wrapper might actually be better, since we're allowing the stream wrapper to create managed files; once a session times out, the "file" will be gone, but File Entity will persist. I'd guess that the entity manager won't be happy if we try to delete the files. So it might be neater to use State for everything, since State will stick around. Makes the demo easier to uninstall (as we probably should with this demo). Your thoughts on this?

I'll update the patch and regenerate an interdiff once I finish with the documentation changes and the rest of the items in your list.

I've updated github. Just about everything that needed a doc fix is fixed. Most things that needed to get dropped (the "config download" business was an out-and-out typo) got dropped. Things that needed to be burned with fire are now a smoldering ruin, save for uses of $_SESSION that affect the session:// demo stream wrapper, since I don't have a working version that does not talk to the magic global yet.

I'll take another stab at the session abstraction stuff. Also: we can use state data instead of session data, and again, I think that may be a better solution.

Near publishable version. The highlights:

• Direct use of $_SESSION is entirely eliminated from both the stream wrapper and the app files.
• Improved documentation throughout.
• Code has been linted with drupalcs.
• Since going from the "manipulate $_SESSION via references" to a "session reference through API only" was a large change, there is a helper class to manipulate the session data, and two new PHPUnit tests to cover that new class, and the stream wrapper class. I needed the tests in order to debug the new session object code, and it's pretty high tech: some serious Prophecy mocking, for one thing. It's not the sort of thing to stress in the example, but given how hard the code was to change before I created the new tests, we need to keep them. Might be worth putting a README in one of the directories to explain that no, you can Drupal w/o messing with some of this stuff.

The code works and runs clean. You'll probably need some changes here and there, but I think this code is at least as good as the D7 module I'm replacing.

Patch and interdiff (from my v3) enclosed.

Changing status so we can wake up the testbot.

Looks like the new tests need @group annotations.

Fixed with this patch, I hope.

Not sure why this one failed. The Jenkins log says this:

06:12:24 PHP Fatal error:  Trait 'Drupal\Tests\file_example\MockSessionTrait' not found in /var/www/html/modules/examples/file_example/tests/src/Kernel/StreamWrapperTest.php on line 34
06:12:24 

The only difference between the testbot's environment and mine seems to be PHP version (bot's running 5.5, I'm running 5.6). I'll try to do 5.5 locally to see if I can replicate this.

Tried PHP 5.5. I still pass locally.

Turns out that the problem is specific to running these phpunit tests under run-tests.sh. I have a question up on Drupal Questions about this. If anyone has insight on why run-tests.sh has trouble with tests that run fine under phpunit directly, much obliged.

@Berdir answered the question on Drupal Answers:

I'm not sure but you might not be able to have a non-test class in the tests/src location, try moving that to module/src/... and change the namespace accordingly (Drupal\module...

This indeed fixes the problem. The testbot should be happier this time.

Count me puzzled. I'm:

• Using the same version of PHP (5.5)
• Running from a sub-directory.

The PHPUnit tests run fine, but the Simpletest fails exactly as the patch from #20 failed:

debug: [Debug] Line 120 of modules/examples/file_example/src/Tests/FileExampleTest.php:
Processing button=Unmanaged using PHP, scheme=session, dir=session://cgwRoGymvR, file=session://cgwRoGymvR/ysyM2fUJWQGTNLJQIXsu8e98jY5zPQ.txt

debug: [Debug] Line 131 of modules/examples/file_example/src/Tests/FileExampleTest.php:
Button Text: Saved file as

debug: [Debug] Line 138 of modules/examples/file_example/src/Tests/FileExampleTest.php:
Name of output file: session://cgwRoGymvR/ysyM2fUJWQGTNLJQIXsu8e98jY5zPQ.txt
fail: [Other] Line 148 of modules/examples/file_example/src/Tests/FileExampleTest.php:
File contents matched.
Value 'syxhqh1eCfbaI4TfakLj3zP27o4w7N' is equal to value '

Not Found
The requested URL /checkout/example/file_example/files was not found on this server.

'.

The equivalent part of the script is:

        // Click the link provided that is an easy way to get the data for
        // checking and make sure that the data we put in is what we get out.
        if (!in_array($scheme, array('private', 'temporary'))) {
          $this->clickLink(t('this URL'));
          // assertText give sketchy answers when the content is *exactly* the contents of the
          // buffer, so let's do something less fragile.
          // $this->assertText($content);
          $buffer = $this->getTextContent();
          $this->assertEqual($content, $buffer, "File contents matched.");
        }

The trouble occurs at the "clickLink()" line; this works outside of the bot, but not on the bot.

I need to see what's happening on the bot. I'd guess that if the server process used by the bot keeps an error log, that there's some error I need to see there. But without that, I'm completely in the dark.

I need help with the bot, for certain.

Did you try running this with a high concurrency? like, say, 32 like the bots run? Im wondering if this is a result of there being an unspoken dependency Intra-test and that one test relies on the fact that some things are still leftover in the session variable, which they would have entirely different session variables if accessed concurrently.

@Mixologic -- I'm still using the standard testbot in the output you see above. So whatever drupalci gives me when I upload the patch and set "Needs review" is what I get.

My tests are self-contained, and I'm certainly assuming that things get torn down. But the test that's giving me trouble cycles over 4 different buttons on a form, for four different kinds of stream wrapper schemes, so it's not impossible that certain things are left over.

The question here, though, is why the testbot trips over these things when doing run-tests.sh locally does not have this problem. I'm not sure how I'd figure that out.

I am getting set up with a Vagrant hosted drupalci testbot if there's anything you want me to try.

Thanks for all the dilligence on the testbot, @Torenware and @Mixologic. I'd wager that the fault lies in the example, not the testbot, though it'd be nice to prove it one way or another.

In the mean time, here's some review:

1. +++ b/file_example/file_example.services.yml
   @@ -0,0 +1,18 @@
   +  file_example.stream_wrapper:
   +    class: Drupal\file_example\StreamWrapper\FileExampleSessionStreamWrapper
   +    arguments: ['@request_stack']
   

   Can the wrapper take DI arguments or not? If it can, just use @session. If not, remove arguments here.

2. +++ b/file_example/src/StreamWrapper/FileExampleSessionStreamWrapper.php
   @@ -0,0 +1,890 @@
   +class FileExampleSessionStreamWrapper implements StreamWrapperInterface, ContainerInjectionInterface {
   ...
   +  public function __construct() {
   +    // Dependency injection will not work here, since stream wrappers
   +    // are not loaded the normal way: PHP creates them automatically
   +    // when certain file functions are called.  This prevents us from
   +    // passing arguments to the constructor, which we'd need to do in
   +    // order to use standard dependency injection as is typically done
   +    // in Drupal 8.
   +    $this->requestStack = \Drupal::service('request_stack');
   +    $helper = $this->getSessionWrapper();
   +    $helper->setPath('.isadir.txt', TRUE);
   +    $this->streamMode = FALSE;
   +  }
   ...
   +  public static function create(ContainerInterface $container) {
   +    $request_stack = $container->get('request_stack');
   +    return new static($request_stack);
   +  }
   

   So since DI won't work here, let's get rid of create(), and not implement ContainerInjectionInterface.

   Also, we don't need the request stack, we need the session. So in __construct(), just grab \Drupal::service('request_stack')->getCurrentRequest()->getSession();

3. +++ b/file_example/src/StreamWrapper/FileExampleSessionStreamWrapper.php
   @@ -0,0 +1,890 @@
   +  protected $requestStack;
   ...
   +  public function getSessionWrapper() {
   +    return new SessionWrapper($this->requestStack);
   +  }
   

   Again, we don't need the request stack. Make a protected $session property.

4. +++ b/file_example/src/StreamWrapper/FileExampleSessionStreamWrapper.php
   --- /dev/null
   +++ b/file_example/src/StreamWrapper/MockSessionTrait.php
   
   +++ b/file_example/src/StreamWrapper/MockSessionTrait.php
   @@ -0,0 +1,101 @@
   +trait MockSessionTrait {
   

   This should be in with the tests.

5. +++ b/file_example/src/StreamWrapper/SessionWrapper.php
   @@ -0,0 +1,247 @@
   +  public function __construct(RequestStack $request_stack) {
   +    $this->requestStack = $request_stack;
   +    $this->storePath = '';
   +  }
   

   Inject the session instead of the request stack.

6. +++ b/file_example/src/StreamWrapper/SessionWrapper.php
   @@ -0,0 +1,247 @@
   +  protected function getSession() {
   +    return $this->requestStack->getCurrentRequest()->getSession();
   +  }
   

   Since this method is protected, now we don't need it since we're going to inject the session instead of the request stack.

7. +++ b/file_example/src/Tests/FileExampleTest.php
   @@ -0,0 +1,186 @@
   +    $this->priviledgedUser = $this->drupalCreateUser($permissions);
   

   Really not a fan of this pattern (anti-pattern, really) where we always have a logged-in user with superpowers from setUp() for all tests in the test class.

   Either turn it into a factory method so other tests can use it, or inline it with the test method.

8. +++ b/file_example/src/Tests/FileExampleTest.php
   @@ -0,0 +1,186 @@
   +   * t() no longer returns a string, but is used heavily in this test in contexts where it
   +   * is important that it really return a string, and not TranslatableMarkup.  We substitute
   +   * our own implementation that will hopefully be localizable, but will not have this problem.
   +   */
   +  protected function t($string, $args = [], $options = []) {
   +    return (string) t($string, $args, $options);
   +  }
   

   We can just not use t() in tests. It's the standard for core, and is reasonable.

Can the wrapper take DI arguments or not? If it can, just use @session. If not, remove arguments here.

Turns out it can't, so I've removed the injection of the argument here.

So since DI won't work here, let's get rid of create(), and not implement ContainerInjectionInterface.

Also, we don't need the request stack, we need the session. So in __construct(), just grab \Drupal::service('request_stack')->getCurrentRequest()->getSession();

I've removed create() and the use statements this required.

As for the request stack -- yeah, we actually do need it. The session is not injectable, it turns out ( see the change record and also the issues connected with it), and the SessionManager service, if it's still in Core, is on its way out. Given the way we're manipulating the session object, we also need to do exactly as the change record does, and do the read/write on it as atomically as possible. We really can't stash it at all. So it's fetch the session data, change it, and put it directly back. The is the price of no longer being able to keep references into the $_SESSION array.

That leaves why to use RequestStack and not Request. You'll see that core always injects the former, and never the latter. The explanation is in Symfony blog (New in Symfony 2.4: The Request Stack: the Request turns out to be a flaky object, and if you want to get its correct state, you need go through RequestStack post Symfony 2.4. So if you want to talk to the session, you need to drag a RequestStack around to get at it.

This should be in with the tests.

Originally, MockSessionTrait.php was indeed in tests/. It had to come out, since run-tests.sh will not find traits used in PHPUnit tests unless they are in the module (and the the module's test) namespace. This I learned from @Berdir: http://drupal.stackexchange.com/questions/178831/error-running-a-phpunit....

It might be a better idea to move it into a "Utilities" directory, but yeah, it's gotta be under src/ and not tests/src.

Really not a fan of this pattern (anti-pattern, really) where we always have a logged-in user with superpowers from setUp() for all tests in the test class.

Either turn it into a factory method so other tests can use it, or inline it with the test method.

The required superpowers are very specific to this test, so it made sense to inline it. This I did.

We can just not use t() in tests. It's the standard for core, and is reasonable.

I totally agree. I just brought this forward from the D7 version of the example. It would have worked fine if t() still returned string and not TranslatableMarkup, but since it now does, passing the return of t() to some of the assert functions has unpredictable results. So yeah, nuke it, burn it, and hide the ashes.

A bit of background, finally, on why I'm paying all this attention to the testbot: my patches have been running clean even under pretty extreme circumstances. I was originally running my PHPUnit stuff directly under ../vendor/bin/phpunit, but found that the testbot does two things that can break phpunit tests:

1. It runs everything under a subdirectory of DOCUMENT_ROOT, to make sure there are no dependencies on being at the top level. So now, I'm doing that as well.
2. Test discovery and the autoloader works a little differently, which bit me twice: first when I found that run-tests.sh insists that all tests use the @group annotation, and second when I discovered that the trait I used to factor the session test code would not be found if it was in the Drupal/Tests namespace.

So I fixed all this, and discovered that 3 asserts still fail only in the testbot. I don't have a clue why this is happening, and after @Mixologic spent an hour trying to figure it out, neither does he. Short version: retrieving the content of one of our fake "session files" via FileDownloadController::download() (which is how private:// and temporary:// work) fails only on the session:// URIs, and it fails only on the testbot. Which, well, sucks.

I'm tempted to simply remove the assert causing the trouble, since at this point, it's not at all clear why the testbot is having trouble. My personal theory is that the session is not "fetchable" on some corner cases. But without any visibility into the testbot, who the hell knows, really.

In any case: if these approaches work for you, I'll spin up another patch. I'm actually pessimistic that that testbot will pass it unless that one assert goes out of the test.

Letting the testbot have another whack at it.

Same testbot failure as previously: getting a 404 on retrieving session "files".

Test implementation of the stream wrapper using State instead of Session. Not ready for human review (class/variable names need to change, and accompanying verbiage), but I want to see if the testbot prefers this implementation.

Failure does not relate to the use of session. A clue, maybe?

Originally, MockSessionTrait.php was indeed in tests/. It had to come out, since run-tests.sh will not find traits used in PHPUnit tests unless they are in the module (and the the module's test) namespace.

So PHPUnit tests are supposed to have a root-level bootstrap.php file that all tests can use to load their special classes/traits, etc. That's true of PHPUnit in general and not just Drupal or run-tests.sh.

Drupal has a bootstrap.php at tests/bootstrap.php.

That bootstrapper registers autoloading namespaces for tests it's running.

So some other piece of the implementation is broken.

But naturally the Drupal solution is to just put classes in the wrong place.

OK. Done venting. For now. :-)

As for State instead of Session: That makes the file system per-site instead of per-user, but I don't really care. :-)

I think there are some caveats around session that make it very delicate for contrib use. I was trying to figure out how to change a cookie earlier with no good luck.

So some other piece of the implementation is broken.

That's how I read it too. My impression is that folks know this is broken, but in the scheme of things, they're letting it slide. But yeah, MockSessionTrait.php belongs in the test/src hierarchy. It would be, like the NORMAL thing that it would work from there :-)

So, yeah, hear your rant, call it. Might even raise it.

The state test indicated that our using the session was not the reason the bot decided to hate on me. So I've got another votive offering for the bot. If it passes (it might even, based on some things I changed), then I'll put up an official patch.

If we did want to use a State based implementation, the main virtue is that the demo creates managed files, which live longer than the SESSION that backs them. If we used State, then you wouldn't have the file entity definitions outlive the file data. But that's minor. If we wanted to do it, it's a pretty simple change to what we have already, since the SessionWrapper stuff works as well for State as it does for Session.

So I'm throwing one more shrimp patch on the barbee. Let's see if this mofo does what it needs to do. If it does, then, well, halleluyah.

Well, look at that. Damn thing's running green.

Official patch and interdiff.

Main change was discovering that Core uses slight of hand path processors to make its menu-tail type paths work. I now do the same. Also, I eliminated a warning related to chmod(); turns out that the stream wrapper needs to do things a little differently than I thought.

Some doc changes as well to explain this stuff.

OK, I've pleased the Robot God, and we have a working patch.

Passing the damn bot probably introduced a bit more complexity, but the extra stuff is both useful and under-documented. If anyone's explained how to do menu-tails in D8, I haven't seen it.

After some thought, an idea as to how to handle this issue.

The File API itself is fairly simple, and I think the file_example module should be relatively simple. The complexity in this patch mostly relates to the stream wrapper, which is a lot harder to do in D8 than it was in D7. But there really isn't that much of a dependency between the stream wrapper related stuff and the rest of the module.

What might work here is to split the patch into two separate example modules, file_example and stream_wrapper_example. I'd do it like this:

• The file_example's main form will test what stream wrappers are available, and list them in the description part of the form.
• If the stream_wrapper_example module is enabled, it will appear in the list of available schemes, w/o any further modification of the file_example module.
• The test for file_example will put stream_wrapper_example as one of the modules to load, so the standard test by the testbot will cover what it covers now.
• The session stream wrapper and its two phpunit tests will move into the new module, along with the various services needed to bring it up.
• The new module will have a simple controller type page that suggests you enable the file_example module to play with it.

Stream wrappers are an advanced topic anyway, so we push the complexity into the advanced example, and leave the file_example clean and simple.

I'd keep both modules under this patch, since I think the FileExampleTest is a good torture test of the stream wrapper code, and we want the two modules to be built to work well with each other.

But naturally the Drupal solution is to just put classes in the wrong place.

OK. Done venting. For now. :-)

Although I made a bit of progress on that store: see #2605664: [Needs change record updates] Align TestDiscovery and bootstrap.php's non-permissive loading of PSR-4 namespaces for traits. This might even get in for 8.0.0.

Looks like there were !placeholders in the example. These are now removed.

So are we still considering splitting out to two modules?

@Mile23 -- your call. It probably makes sense, since the stream class is high tech, and file operations should be more accessible.

In any case, looks like I'll need to see why the tests are no longer testing. E*U((#@ Deja Vu All Over Again.

First, I need to get this working again.

Turns out the problem was due to a change recorded as Many methods for generating URLs and links deprecated; EntityInterface::toUrl() and EntityInterface::toLink() added. This broke both the URL building code and the t() statements that built the embedded links.

New code here doesn't trust the standard routines to do the right thing, 'cause they don't: the remaining URL creation APIs are not stream wrapper aware. The solution is to use the StreamWrapperManager to get at the external URL building code in that interface, which is probably what Core should do, but does not. This works, and is simpler than my previous code as well.

It is also no longer possible to use LinkGeneratorTrait::l() to embed a link in a localizable string. I will not rant about this BUT I MUST!! I MUST!!! Read the relevant issues attached to the CR, and well, weep. I do things according to the CR, but I doth protest.

New patch and interdiff uploaded. I'll start separating this into two example modules once the combined module passes ROBOT DEATH MATCH.

I'm transferring work between laptops; looks like I reused my patch numbers. Not to worry; the lastest path #9 works.

First working version split into the File Example and Stream Wrapper Example modules. This is a good time to get some feedback on the work.

Thanks for splitting them out.

Since they don't seem to be dependent on each other, let's make a separate issue for the stream wrapper example. Here's a patch with only file_example.

Created #2638290: Create stream_wrapper_example

I think there's currently a dependency (easy to remove) where the file example's test brings up the stream wrapper module as one of the streams it tests. I'll remove it and see if that makes the newly separated example test clean.

Also: looks like the trait loading issue is starting to get attention from Alex and the gang. Once it's in, I'll factor the stream wrapper tests accordingly.

Oh, if there's a dependency (and it's worth keeping) then just ignore #90.

Just make it a hard dependency in file_example's info.yml.

So I'm off in the weeds on this right now because d.o's testbot demands that we test against 8.1.x-dev. Just ignore that for now, because I think the APIs haven't changed that much.

The difference is pretty trivial; see the enclosed interdiff. Might be worth re-adding the dependency later, once streamwrapper_example lands. As is, the SWE in #2638290: Create stream_wrapper_example tests green. We can get this one green as well, and recreate the dependency as a follow-up issue, once both examples are in the tree.

I'd really like to have the changes in #2605664: [Needs change record updates] Align TestDiscovery and bootstrap.php's non-permissive loading of PSR-4 namespaces for traits so that we can then commit things the right way (traits for tests in the tests directory), but it looks like that has fizzled out.

Therefore let's just do the workaround, document it as a @todo, and make a follow-up to move it once the fix happens.

Re-running the test now...

Reviving this one in time for DrupalCon. This patch fixes most issues with drupalcs (some things it dislikes are things I want to keep), and adds the item to the Toolbar menu. The module no longer depends on the Stream Wrapper Example, but the form will use the "session" wrapper if that module is activated.

This appears to have failed because we're testing along with the rest of Example; #2690403: Fatal error: Class 'Drupal\field\Tests\FieldUnitTestBase' not found.

03:55:40 Fatal error: Class 'Drupal\field\Tests\FieldUnitTestBase' not found in /var/www/html/modules/examples/field_permission_example/src/Tests/FieldNoteItemTest.php on line 22
03:55:40 HTTP/1.1 200 OK
03:55:40 Content-Type: application/vnd.docker.raw-stream
03:55:40 
03:55:40 
03:55:40 Error
03:55:40 Received a failed return code from the last command executed on the container.  (Return status: 255)
03:55:40 Execution Error
03:55:40 Error encountered while executing job build step execute:testcommand
03:55:40 Checking console output
03:55:40 /usr/local/jenkins/jobs/default/builds/116870/log:
03:55:40 PHP Fatal error:  Class 'Drupal\field\Tests\FieldUnitTestBase' not found in /var/www/html/modules/examples/field_permission_example/src/Tests/FieldNoteItemTest.php on line 22
03:55:40 Build step 'Jenkins Text Finder' changed build result to UNSTABLE
03:55:40 Recording test results
03:55:40 ERROR: Step ?Publish JUnit test result report? failed: No test report files were found. Configuration error?
03:55:40 Finished: FAILURE

I'm going to try to test w/o the upstream integration. Is there any way to do that?

This appears to be the issue from #2690403: Fatal error: Class 'Drupal\field\Tests\FieldUnitTestBase' not found. Looks like we can fix that, which will fix this.

Hi @Torenware,

As mentioned by @Mile23 on #2661834: Core uses hyphen for paths, not underscores, I have added a test against Drupal 8.1.x which results as Pass.
Tested functionality on local, everything is working fine.

Review Results : Pass
Drupal : 8.1.0
MySQL : 5.5.42
PHP : 5.6.10

Whew. Man we need to get this out the door.

Still a few items to address, mostly relating to coding standards, but also some code stuff.

I think if we can get these changes we can then get it released and file follow-ups if there are any.

1. +++ b/file_example/file_example.module
   @@ -0,0 +1,95 @@
   + *   * Using special "stream" URIs like public://, private://, and temporary://.
   + *     Drupal has good support for this PHP language feature.  You can implement
   + *     new file schemes as well; see the Stream Wrapper Example for how to do that.
   + *     If you enable the stream_wrapper_example module, you can use it together
   + *     with the File Example to test how a custom stream works.
   + *
   + * To demonstrate all of this, the File Example implements a form that lets you
   + * play with files. Read src/Form/FileExampleReadWriteForm.php to see demostrations
   + * of the various File API functions you will want to use in your code.
   

   Needs to wrap at 80 chars.

2. +++ b/file_example/file_example.module
   @@ -0,0 +1,95 @@
   + * Control access to private file downloads and specify HTTP headers.
   

   Should be 'Implements hook_file_download().'

   https://www.drupal.org/coding-standards/docs#hookimpl

3. +++ b/file_example/file_example.routing.yml
   @@ -0,0 +1,9 @@
   +# Main page for our example.   ¶
   ...
   +
   

   Too much whitespace.

4. +++ b/file_example/src/Form/FileExampleReadWriteForm.php
   @@ -0,0 +1,857 @@
   +/**
   + * @file
   + * Contains \Drupal\file_example\Form\EmailExampleGetFormPage.
   + */
   
   +++ b/file_example/src/Tests/FileExampleTest.php
   @@ -0,0 +1,182 @@
   +/**
   + * @file
   + * Contains Drupal\file_example\Tests\FileExampleTest.
   

   New coding standards: Don't use @file for 'contains' if there's a namespace declaration.

   https://www.drupal.org/coding-standards/docs#file

5. +++ b/file_example/src/Form/FileExampleReadWriteForm.php
   @@ -0,0 +1,857 @@
   +use Symfony\Component\HttpFoundation\RequestStack;
   +
   +
   +/**
   

   Should be one blank line.

6. +++ b/file_example/src/Form/FileExampleReadWriteForm.php
   @@ -0,0 +1,857 @@
   +  public function __construct(StateInterface $state, FileSystemInterface $file_system,
   +                              StreamWrapperManagerInterface $stream_wrapper_manager,
   +                              ModuleHandlerInterface $module_handler, RequestStack $request_stack) {
   

   Put all these arguments on their own line.

7. +++ b/file_example/src/Form/FileExampleReadWriteForm.php
   @@ -0,0 +1,857 @@
   +   *    * @param array $form
   +   *   An associative array containing the structure of the form.
   ...
   +   *    * @param array $form
   +   *   An associative array containing the structure of the form.
   ...
   +   *    * @param array $form
   +   *   An associative array containing the structure of the form.
   

   Too many * as a consequence of copypaste.

8. +++ b/file_example/src/Form/FileExampleReadWriteForm.php
   @@ -0,0 +1,857 @@
   +   * Note this does NOT clear any managed file references in Drupal's DB.
   +   * Perhaps we should do this as well.
   

   Turn this into @todo so we see it later.

9. +++ b/file_example/src/Tests/FileExampleTest.php
   @@ -0,0 +1,182 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   *  Note that we enable the stream_wrapper_example so we can also
   +   *  test if the test code handles a custom stream wrapper as well.
   +   *
   +   * @var array
   +   */
   +  public static $modules = array('file_example', 'file');
   

   We don't seem to be enabling stream_wrapper_example in this test. Fix the docs.

10. +++ b/file_example/src/Tests/FileExampleTest.php
    @@ -0,0 +1,182 @@
    +  public static $modules = array('file_example', 'file');
    

    file_example should declare its dependency on file, and then this test should just declare a dependency on file_example.

11. +++ b/file_example/src/Tests/FileExampleTest.php
    @@ -0,0 +1,182 @@
    +  /**
    +   * SimpleTest user widget, since we need to log in to handle permissions.
    +   *
    +   * @var \Drupal\user\Entity\User
    +   */
    +  protected $priviledgedUser;
    

    We never use $priviledgedUser outside of testFileExampleBasic(), and it's a local variable there. Remove this property.

12. +++ b/file_example/src/Tests/FileExampleTest.php
    @@ -0,0 +1,182 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function setUp() {
    +    parent::setUp();
    +  }
    

    No-op. Remove.

Let's see if this does the trick here.

Does the trick ftmp. :-)

I did some coding standards changes, and turned the stream_wrapper_example part of the form element into a @todo.

See attached interdiff.

Huge thanks to @Torenware for being patient and getting it done.