The code seems to be working, but should be reviewed before creating an official release.

There is an unfortunate need to use the reflection class to reset a protected property, but otherwise I feel like the code is pretty clean.

CommentFileSizeAuthor
#9 3049150-9.patch24.64 KBpwolanin
#8 3049150-8.patch24.64 KBpwolanin
#8 Screen Shot 2019-04-18 at 17.11.19.jpg96.5 KBpwolanin
#6 3049150-6.patch4.17 KBpwolanin
#5 3049150-5.patch4.37 KBpwolanin
#3 3049150-3.patch2.74 KBpwolanin

Comments

pwolanin created an issue. See original summary.

pwolanin’s picture

pwolanin commented
Issue summary: View changes

To prove the tests do something, comment out line 148 in \Drupal\views_streaming_data\StreamingViewExecutable:
// $efr->setValue($obj, NULL);

then there is a fail:

Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-"node 5","No, I like Llamas, Hazard",Happy!'
+,,'
pwolanin’s picture

pwolanin commented
Status: Active » Needs review
StatusFileSize
new 3049150-3.patch2.74 KB

Here's a patch for initial fixes

pwolanin’s picture

pwolanin commented
Category: Support request » Task
pwolanin’s picture

pwolanin commented
StatusFileSize
new 3049150-5.patch4.37 KB

Plus a couple more that Erich had mentioned.

pwolanin’s picture

pwolanin commented
StatusFileSize
new 3049150-6.patch4.17 KB

Turns out we need PHP 7.1 for nullable return typeint, so taking that out.

ebeyrent’s picture

ebeyrent commented
  1. In /src/Plugin/views/style/StreamingJsonSerializer.php on line 85, it looks like the comment may not be correct - it looks like it's always adding a comma and a newline after each row. The final row should not end in a comma, I think.
  2. In this same method, isn't $traversable set to a chunk of 50 rows? If so, are we wrapping each chunk in [ ]?
  3. In /src/Plugin/views/style/StreamingCsvSerializer.php::defineOptions(), the delimiter default value should maybe be set to $this->delimiter, not ','
  4. In /src/Plugin/views/style/StreamingCsvSerializer.php::extractHeaders() on line 192, the coalesce operator could be used to shorten the statement.
  5. In /src/Plugin/views/display/StreamingDataExport.php::optionsSummary(), it looks like we have a dependency on views_ui on line 351. Is this okay?
  6. In /src/Plugin/views/display/StreamingDataExport.php::overrideApplies(), strict equality should be used for checking the array_intersect() !== empty array
  7. In /src/Plugin/views/display/StreamingDataExport.php::buildResponse(), might be nice to be able to override the filename
  8. In /src/StreamingViewExecutable.php::render(), the Note in the docblock is really awkward and could be clearer
  9. In /src/StreamingViewExecutable.php::execute(), we discussed removing the call to _postExecute() and invoking the views_post_execute hook. Are we positive we want to leave these in place?
  10. In /src/StreamingViewExecutable.php::getIterator(), we've got chunk size defaulted to 50, which is also hardcoded in another class. This should maybe be a constant?
  11. In /src/ViewExecutableFactory.php::get(), is there a way to pass in the StreamingViewExecutable instance via DI?
pwolanin’s picture

pwolanin commented
StatusFileSize
new Screen Shot 2019-04-18 at 17.11.19.jpg96.5 KB
new 3049150-8.patch24.64 KB

Thanks @ebeyrent

  1. The code is correct, but maybe the comment is confusing. It adds a comma before each row after the 1st. Tweaked the comment. Added test to verify JSON can be parsed and matches core once parsed.
  2. So, this is working via a generator. The chunk of e.g. 50 results is set as an array on $this->view->result after the foreach loop is entered before the first result is yielded.
  3. fixed
  4. Actually can't use ?? here becuase the value is an empty string usually, but it could also be not set so empty() is better as-is
  5. Maybe fine since the options summary is for the UI, but replaced with Unicode::truncate()
  6. fixed
  7. Yes, would be a good addition but postponing for now
  8. The comment wasn't really relevant to this class, so deleted it.
  9. It's working fine with these in place now (not really any core implementations) so I prefer to leave them unless we hit a bug
  10. add StreamingDisplayInterface::DEFAULT_CHUNK_SIZE
  11. No, the whole point of this factory is to create that instance. We could define the class name in the services file, or as a method parameter, but I don't have any use case for that

We also discussed adding a message to the views UI preview, which I did.

Views UI message

pwolanin’s picture

pwolanin commented
StatusFileSize
new 3049150-9.patch24.64 KB

Oops, need php 7.1 for public const. Changed to just const.

ebeyrent’s picture

ebeyrent commented

I think this looks really good, thanks for the detailed responses to my feedback!

pwolanin’s picture

pwolanin commented
Status: Needs review » Fixed

Thanks, committed and going to make a 1.0

  • pwolanin committed 632727a on 8.x-1.x 
    Issue #3049150 by pwolanin, ebeyrent: Review code before first official...

Status: Fixed » Closed (fixed)

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