description of drupalGetTestFiles() is vague and has a typo; simpletest_generate_file() needs help too

Same in WebTestBase::drupalGetTestFiles() in Drupal 8.

https://api.drupal.org/api/drupal/core!modules!simpletest!src!WebTestBas...

Looking at the code, it appears that the files go into the public files directory (which is set to something special during tests). Some files are generated, but some come from core/modules/simpletest/files.

I have no idea about that. Anyway I do not think we need to document where the files are, because the return value gives you their paths.

But why does a developer need to know this information ahead of time? That seems rather pointless. The point of this function is to return a list of the files, abstracting away the details so the developer doesn't need to know where they are, because this function lists them, right??!?

I'm resurrecting this issue.

What needs to be done, in my opinion:

a) Fix the grammar/typos in the method documentation.

b) Fix the documentation so it is accurate: it should say that it is generating files. The current docs imply it is just "getting a list", not that it is creating new files.

On it.

Seriously, the $size parameter is almost useless, and used in only one, single test in Core. The problem is you need to know exactly what sizes are available in the test files. I'm listing them for clarity (and because checking the tests/files folder, as suggested, is a pain), but it's really not intuitive. It would be better return files that are larger than the passed size, or just leave the size parameter out completely (like marking it as deprecated, or discouraged).

Anyway, patch attached.

OK... looks pretty good!

That list of file sizes could be more concise. Maybe something like:

 * binary: 1, 45, 345
 * html: 22, 55, 66

(I totally made up those numbers, but get the idea?)
I think that would be better, because the list is kind of long and in kind of a random order... I mean it is in order by size but probably for a given test you'd be interested in just one type of file anyway, and it's hard to scan to see what sizes are actually available for one particular type. Thoughts?

Other than that, looks fine to me.

Good point. On it.

Much nicer!

A couple of very minor points:

1. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -448,16 +448,45 @@ protected function findBlockInstance(Block $block) {
   +   *   filter the returned list by size. The default generated files can be of
   +   *   the following sizes (in bytes):
   +   *   - binary: 16, 256, 1024, 2048 and 20480
   

   "can be" seems a bit odd to me. Maybe just say:

   The files generated by this function are of the following sizes...

2. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -448,16 +448,45 @@ protected function findBlockInstance(Block $block) {
   +   *   - binary: 16, 256, 1024, 2048 and 20480
   ...
   +   *   - image: 125, 183, 964, 1831, 1901 and 39325
   

   Technically we require a serial comma in Drupal docs, so:

   ... 2048, and 20480

How's this?

Looks good to me, thanks!

Overall this looks great. Two comments:

1. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -448,16 +448,45 @@ protected function findBlockInstance(Block $block) {
   +   * The first time this method is called, the class will generate a bunch of
   

   The expression "a bunch" is informal. Could we say "numerous" or "several"... or better, exactly how many?

2. A lot of the information added to this docblock actually describes how simpletest_generate_file() works. That function's documentation is also rather thin. I think it's fine to have the big picture documentation in this method, but if we improve the related function's documentation, then we can reference that.

Good idea! So I guess the plan would be:

a) Put the docs about generating files into simpletest_generate_files().
b) In drupalGetTestFiles(), mostly just say it returns the files generated by simplest_generate_files(), plus anything else that someone drops in there.

Followed jhodgdon's advice above, divided documentation between the two files. Patch and interdiff attached.

Thanks for the new patch! I don't think this is quite what we were after, however.

The function simpletest_generate_file() takes as input the name of the file to create. So we shouldn't be putting anything in there about what the file names are; that part comes from the calling method WebTestBase::drupalGetTestFiles().

So. Let's go back to the patch in #13 and also look at the existing docs for simpletest_generate_file()... Let's do this (starting from that patch):

a) Add something to the docs for simpletest_generate_file(), in the @param $type docs, that explains what is actually put into the file for each value of $type: for text it looks like it's random ASCII characters. For binary, it's random characters whose codes are in the range of 0 to 31. For binary-text, it's a random sequence of 0 and 1 values. Also explicitly say what the default value is (binary-text).

b) Add something to the docs for @param $filename on simpletest_generate_file() that explains that .txt is appended to the supplied file name and the file is put in the public file directory.

c) Leave the first line of WebTestBase::drupalGetTestFiles() alone -- it was good in the patch of #13.

d) The first paragraph of the method:

+   * The first time this method is called, the class will generate a bunch of
+   * test files in text and in binary format. These files are stored in
+   * public:// and can be used for tests. It will copy all files in

Change this to say something like: The first time this method is called, it will call simpletest_generate_file() to generate text and binary-text files in the public:// directory. It will also copy all files in ...

The reason for this change is that technically this method is not passing 'text' as the type to simpletest_generate_file(), so it will be generating the default binary-text type of file, not 'text', in:

     // Generate text test files.
      $lines = array(16, 256, 1024, 2048, 20480);
      $count = 0;
      foreach ($lines as $line) {
        simpletest_generate_file('text-' . $count++, 64, $line);
      }

That could be a bug, but fixing it is out of scope for this documentation issue.

e)

+   * All files are prefixed with their type:

Let's change this to say "filenames" instead of "files".

f)

    * @param $size
-   *   File size in bytes to match. Please check the tests/files folder.
+   *   (optional) File size in bytes to match. Defaults to NULL, which will not
+   *   filter the returned list by size. The files generated by this function
+   *   are of the following sizes (in bytes):

Let's change the last two lines to say something like "The files generated and copied by this function..." to be more accurate.

g)

+   * All files are prefixed with their type:
+   * - text-*.txt and text-* (no extension)
+   * - binary-* (no extension)
+   * - html-*.html
+   * - image-*.png, image-*.jpg and image-*.gif
+   * - javascript-*.txt and javascript-*.script
+   * - php-*.txt and php-*.php
+   * - sql-*.txt and sql-*.sql

Looking at simpletest_generate_files() and the files that are currently in core/modules/simpletest/files, this is not accurate. The ones generated will all have .txt extensions, so in this function it will generate text-*.txt and binary-*.txt, so I don't know where that "no extension" idea comes from. And in the HTML line, there is also html-*.txt.

Thanks!

Thanks for the detailed explanation, it is much appreciated. I think I understand all of your points and I will attempt a proper fix as soon as possible.

Hi @jhodgdon,

So sorry it took me so long to get back on this but I have the whole day to work on it and would love to get this issue closed! I'm new at contrib though and have a question if you don't mind: You asked me to modify the docs for simpletest_generate_file(). Should that go within the function: "protected function drupalGetTestFiles..." (line 499 of Web TestBase.php) or should it go above that function with the other docs?

Thanks!
Tim

Hi @jhodgdon,

Sorry, never mind. I see that I should be putting this in the simpletest.module file, around line 490! My bad. :(

Tim

Hi @jhodgdon and @xjm, here is my latest effort for this patch and interdiff. I tried to follow jhodgdon's excellent advice above as closely as possible, but please note that I added a couple things myself in WebTestBase.php:

Line 466 - added "and have appropriate extensions:"
Line 470 - added serial comma after "image-*.jpg"

I recreated the patch to fix errors.

Thanks! This is very close. Just a few cosmetic things to fix:

1. +++ b/core/modules/simpletest/simpletest.module
   @@ -490,13 +490,17 @@ function simpletest_classloader_register() {
   + *   The name of the file, including the path. The suffix ".txt" is appended
   + *   to the supplied file name and the file is put into the public:// files directory.
   

   Documentation lines need to wrap as close to 80-character lines as possible, without going over. The first line is too short (at least, I think "to" will fit there), and the second is too long.

2. +++ b/core/modules/simpletest/simpletest.module
   @@ -490,13 +490,17 @@ function simpletest_classloader_register() {
   + *   If $type is "text", random ASCII characters are inserted into the file.
   + *   If $type is "binary", random characters in the range of 0 to 31 are inserted into the file.
   + *   The default value is "binary-text", and a random sequence of 0 and 1 values are inserted into the file.
     *
   

   These lines also need rewrapping into one paragraph of less than 80 character lines.

   I think also for "binary" it should say "...random characters whose codes are in the range..." ?

   And we can shorten the 'default' bit, something like:

   If $type is "binary-text" (default), a random ...

   [note: no word "and" in the binary-text sentence]

3. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -456,16 +456,44 @@ protected function findBlockInstance(Block $block) {
   +   * The first time this method is called, it will call simpletest_generate_file()
   +   * to generate text and binary-text files in the public:// directory.
   +   * It will also copy all files in core/modules/simpletest/files to public:// as well.
   +   * These contain image, SQL, PHP, JavaScript and HTML files.
   +   *
   

   Re-wrap.

   Also I think we don't need "as well" at the end of the second sentence, since it starts with "... will also". :)

Thanks for the tips, it is helpful to know the proper form and style! Working on this today.

Again, thank you for clearing up the 80 characters in documentation for me. I was doing it by what 'looks' best, I guess from my very old DTP days. :-) So I set phpStorm to show me 80 character columns rather than 120, which made it easy! Attaching the new patch and interdiff now.

NOTE: Please ignore the files above. I named them wrong because I didn't refresh the page before I got the suggested patch name. Rookie mistake. I will re-roll the files and post them with the proper file names. Thanks for your patience!

Latest patch and interdiff, re-rolled and named correctly.

I'm not too hung up on patch names. ;)

Anyway, this is looking good! But unfortunately, I double-checked and it's not quite accurate:

1. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -456,16 +456,45 @@ protected function findBlockInstance(Block $block) {
   +   * simpletest_generate_file() to generate text and binary-text files in the
   

   It looks like the code in the generate files function is actually calling simpletest_generate_file() with 'binary' for the files whose names start with 'binary', and no arg (which defaults to 'binary-text', not 'text') for the files whose names start with 'text'.

   So we should document this correctly. In this line it should say it generates binary and binary-text types, and down below I think we should note that the text-*.txt files are actually binary-text.

   And we should possibly also file a follow-up issue to change this, because it looks like the intention may have been that the "text" files contained random ASCII characters? I do not know.... ???

2. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -456,16 +456,45 @@ protected function findBlockInstance(Block $block) {
   +   *   - binary: 16, 256, 1024, 2048, and 20480
   

   The binary files generated are actually of size 64 and 1024.

3. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -456,16 +456,45 @@ protected function findBlockInstance(Block $block) {
   +   *   - text: 64, and 1024
   

   The binary-text files are actually: 16, 256, 1024, 2048, 20480 -- and again note that these are binary-text not really "text".

Wow, thank you for catching these things! I could have checked more carefully... Working on it now.

Newest patch and interdiff per instructions above. Thanks for the note about patch names. Understood, but I will still try to name them correctly. :-)

NOTE: We will need someone with more experience than I have to determine if a new issue should be created around "it looks like the intention may have been that the "text" files contained random ASCII characters?"

Needs review.

Actually @jhodgdon, I think I get what you're saying about the followup issue. If we can determine that what you said is correct, and text files should contain ASCII characters, I might be able to tackle it.

This issue is my first effort at contrib (and thanks for all the help!) but it would also be very exciting to contribute some actual code to core. What is your recommendation for this?

If this is actually the desired behavior (ASCII characters), could it be as simple as replacing line 518 of simpletest.module: $text .= chr(rand(32, 126)); with some form of $text .= string chr ( int $ascii )? Or maybe even some form of the fprintf() function?

No, what I think needs to be done is that the method in WebTestBase should pass 'text' in as the type, instead of getting the default of 'binary-text'. Right? I think that the function in simpletest.module is clear enough, and now it's actually documented (once we get this patch in). But WebTestBase, in my opinion, is not calling it correctly.

Anyway, the first step there would be to write up an issue with a clear summary of the problem, which I think is that that WebTestBase method is generating binary-text files instead of text files, and it looks like probably text was the intention.

Meanwhile, I believe that the current patch clearly documents the current code. Thanks!

Re the new issue: of course you are right about how to do it, your way sounds much simpler. I will write up the new issue tomorrow morning, and if it's okay with you, PM you the URL so you can see if I did it correctly?

Re this issue: Awesome!! Thanks so much for your help. :-) As I said this is my first contrib and it's actually very exciting! So... what's next for this issue? Do I need to do anything else?

Thanks again,
Tim

Please post the issue URL here in this issue rather than messaging me in IRC. And use the "relationships" section on the issue to mark it as "related" to this issue. Thanks!

And for this issue, you're done unless it gets marked back to "needs work". At least until it is committed to Drupal 8 -- then someone will need to backport the patch to Drupal 7 (but wait until D8 has been taken care of before that, just in case it still is deemed to need work).

Thanks for such clear explanations and help with this issue. I will create the new issue later today or first thing tomorrow, post the URL here, relate it back to this issue, and try to solve it myself!

And I am following this issue of course, so if there is any more work needed I will be alerted and will be happy to do it.

I have created a related issue to force text files to be created as 'text' rather than the default of 'binary-text'

Here is the issue URL: https://www.drupal.org/node/2512210

We need to fix this a bit -- these are not the right sizes for the generated files:

+   *   function are of the following sizes (in bytes):
+   *   - binary: 64, and 1024
+   *   - html: 24
+   *   - sql: 41
+   *   - php: 44, and 47
+   *   - javascript: 57, and 58
+   *   - text: 16, 256, 1024, 2048, and 20480 (these are binary-text files)
+   *   - image: 125, 183, 964, 1831, 1901, and 39325

For binary and text, these numbers are the number of *lines* being generated, not the number of bytes in the file.

Changing 'bytes' to 'lines' in WebTestBase.php, documentation line #488, since this is more accurate after fixing the related issue.

Thanks! But...

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -457,16 +457,45 @@ protected function findBlockInstance(Block $block) {
+   *   (optional) File size in bytes to match. Defaults to NULL, which will not
+   *   filter the returned list by size. The files generated and copied by this
+   *   function are of the following sizes (in lines):
+   *   - binary: 64, and 1024

This is possibly accurate, but not useful. The caller has to pass in the file size in bytes to match, but we only tell them the number of lines and not how many bytes are in each line. How would they figure this out?

Plus I think maybe that the other files (the ones that are copied not generated) are given as file sizes? Unsure...

Anyway I don't think this is a great way to document it. If they have to match file size, we should either tell them the file sizes, or leave this list out entirely.

Good point, seems like too much documentation in this case? I'm thinking of leaving it out since as you said they need to pass the file sizes in. How about this as the last line of that doc block?

* @param $size
* (optional) File size in bytes to match. Defaults to NULL, which will not
* filter the returned list by size.

Thoughts?

Works for me.

Excellent. Here's a patch with "less is more" documentation. :-)

Thanks! This looks fine to me now.

Note that if #2512210: SimpleTest - WebTestBase method creates binary-text files when the intention was to create text files, and text file creation is broken gets committed first, we'll need to come back here and update the docs so that it doesn't say the 'text' files are actually 'binary-text' any more.

Updated summary and added brief beta eval.

#51 looks very good to me, but maybe we should postpone this on #2512210: SimpleTest - WebTestBase method creates binary-text files when the intention was to create text files, and text file creation is broken since that one is also RTBC? Thanks!

The other issue got in. This patch needs a bit of an update now. See comment #52.

Yes, will work on it this afternoon.

I changed "binary-text" to "text" in one place and removed what I consider to be unnecessary documentation on one line. But do you think we should say "ASCII text" instead of just "text?"

ASCII text sounds like a good idea to me. Also one other thought/question:

+++ b/core/modules/simpletest/simpletest.module
@@ -490,13 +490,19 @@ function simpletest_classloader_register() {
+ *   random sequence of 0 and 1 values are inserted into the file.

Do you think we should also put '0' and '1' in quotes here, to avoid it being mistaken for real binary numbers, or is it clear enough because the type is "binary-text"?

Agreed on both ASCII and '0' and '1' in quotes. It will make it more clear. However, to possibly avoid another patch, should I use single or double quotes for the 0 and 1? We are using double quotes for "text" but from a PHP perspective maybe single quotes are better for the numbers?

Generally, our Drupal coding standards prefer single quotes in code, so we should also use them in docs. I would change all of them to single quotes. Thanks!

Makes perfect sense! Happy to do it.

Great!

So I took one more look at this patch and since it's just documentation at this point, I found a few nitpick things to fix to make the docs just a bit better:

1. +++ b/core/modules/simpletest/simpletest.module
   @@ -490,13 +490,19 @@ function simpletest_classloader_register() {
     * Generates test file.
   

   If we're fixing up the docs, can we put "a" in here? :)

2. +++ b/core/modules/simpletest/simpletest.module
   @@ -490,13 +490,19 @@ function simpletest_classloader_register() {
   + *   (optional) The type, for example: 'text', 'binary', or 'binary-text'. If
   

   These are not really "examples", but actually the three allowed values.

   And would this be clearer as a list? Something like:

   (optional) The type, one of:
   - text: The generated file contains random ASCII characters.
   ...

3. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -457,16 +457,37 @@ protected function findBlockInstance(Block $block) {
   +   * JavaScript and HTML files.
   

   We use serial commas in Drupal docs by convention, so needs a comma after JavaScript in this line.

addressed #62

Thanks! Pretty close but:

+++ b/core/modules/simpletest/simpletest.module
@@ -487,16 +487,23 @@ function simpletest_classloader_register() {
+ *     - binary: The generated file random characters whose codes are in the
+ *       range of 0 to 31 are inserted into the file.
+ *     - binary-text: The generated file random characters whose codes are in
+ *       the range of 0 to 31 are inserted into the file.

These two descriptions are garbled.

Thanks for the new patch! But:

+++ b/core/modules/simpletest/simpletest.module
@@ -487,16 +487,23 @@ function simpletest_classloader_register() {
+ *   (optional) The type, one of:
+ *     - text: The generated file contains random ASCII characters.
+ *     - binary: The generated file contains random characters whose codes are
+ *       in the range of 0 to 31 are inserted into the file.
+ *     - binary-text: The generated file contains random sequence of '0' and '1'
+ *       values are inserted into the file.

These are still garbled.

Either say "contains" or "are inserted into the file", but not both. Either way is fine with me.

Incorporated #67 suggestion.

+++ b/core/modules/simpletest/simpletest.module
@@ -487,16 +487,23 @@ function simpletest_classloader_register() {
+ *   (optional) The type, one of:
+ *     - text: The generated file contains random ASCII characters.
+ *     - binary: The generated file contains random characters whose codes are
+ *       in the range of 0 to 31.
+ *     - binary-text: The generated file contains random sequence of '0' and '1'
+ *       values.
  *

OK, now the text looks good, thanks!

Unfortunately (sorry I didn't notice this before), the indentation is two spaces too much. List markers - are lined up with the text above.

Thanks for the patches! I figured I would go ahead and implement #72 above.

Great, thanks! I think this is ready to go in now.

Thanks for reworking this one after the other patch.

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -457,16 +457,37 @@ protected function findBlockInstance(Block $block) {
+   *   (optional) File size in bytes to match. Defaults to NULL, which will not
+   *   filter the returned list by size.

So this parameter is... bizarre. It's especially fussy since you have to use an exact size matching a file generated by simpletest_generate_file()... which doesn't even generate based on size; it generates based on lines and characters! Looking above, it seems a similar concern was raised in #8.

I checked and it is used three times, once directly and twice when passed to the wrapper method in FileFieldTestBase::getTestFile(). Somewhere since #15 we lost the previous docs about what sizes were available... looks like that was done deliberately in #51. I can see the case for that. However, reading the current docs, I don't know how to find out what file sizes there are as we have also lost the prompt in HEAD to check the temporary directory (at least I think that's what it means).

TBH it's code smell and it seems like the parameter should possibly be moved to the calling code, but that's out of scope. I think what we should do is file a followup to deprecate the size parameter and replace the test usages (both beta-eligible changes) since the docs here are an improvement overall.

This issue only changes documentation, so per https://www.drupal.org/core/beta-changes, this can be completed any time during the Drupal 8 beta phase. Committed and pushed to 8.0.x. Thanks everyone for figuring out how this kinda strange API works and documenting it!

Bizarre indeed. Follow-up:
#2534800: \Drupal\Tests\TestFileCreationTrait has mostly unusable file size filter

Also we need to backport these docs fixes to Drupal 7.

Tagging the backport to D7 as a novice task -- just note that not necessarily all of the code will apply (for instance, "core/modules/simpletest/files" is "modules/simpletest/files" in D7.

Prior to backporting we should also manually determine that all of the D8 comments apply to D7 as well.

Here I go ;)