tec: Refactor the export feature #3045

marienfressinaud · 2020-06-09T17:35:10Z

Changes proposed in this pull request:

Even if the issue #3035 seemed pretty simple at a first glance, it was
more complicated than I expected. Because we send CSP headers AFTER
running the controller actions, it means we can't "echo" any content
from the controller. It's in fact a good practice, but it was easier at
the time we developed the feature.

To fix that, the only thing I had to do was to move the print() and
readfile() function into the view. The problem was that we needed to
output the content from the CLI too. Then, things became more
complicated. I decided to extract the export-related methods in a
FreshRSS_Export_Service class, in order to use it from both the
controller and the CLI. It was an opportunity to refactor the whole
feature in order to make it a bit more linear and easy to read.

How to test the feature manually:

Export OPML, labelled, starred, and feeds files (one by one, then several to generate a Zip) from the browser
Run the php cli/export-zip-for-user.php and php cli/export-opml-for-user.php commands
Verify none of the files contain an error and that they can be imported back to FRSS

Pull request checklist:

clear commit messages
code manually tested
N/A unit tests written (optional if too hard)
N/A documentation updated

Additional information can be found in the documentation.

Even if the issue FreshRSS#3035 seemed pretty simple at a first glance, it was more complicated than I expected. Because we send CSP headers AFTER running the controller actions, it means we can't "echo" any content from the controller. It's in fact a good practice, but it was easier at the time we developed the feature. To fix that, the only thing I had to do was to move the `print()` and `readfile()` function into the view. The problem was that we needed to output the content from the CLI too. Then, things became more complicated. I decided to extract the export-related methods in a `FreshRSS_Export_Service` class, in order to use it from both the controller and the CLI. It was an opportunity to refactor the whole feature in order to make it a bit more linear and easy to read. Reference: FreshRSS#3035

marienfressinaud · 2020-06-09T17:36:28Z

app/Controllers/importExportController.php

@@ -720,76 +720,6 @@ private function addFeedJson($origin) {
 		return $return;


The diff concerning this file is a bit complicated to review, I suggest to only read the new file

marienfressinaud · 2020-06-09T17:37:26Z

app/Services/ExportService.php

+		return [
+			"feeds_{$day}.opml.xml",
+			$view->helperToString('export/opml')
+		];


I'm not completely happy with this pattern (i.e. returning an array containing the filename and the content) but it makes the things easier in the controller and the cli

marienfressinaud · 2020-06-09T17:38:30Z

app/Services/ExportService.php

+			$type, '', FreshRSS_Entry::STATE_ALL, 'ASC', -1
+		);
+		$view->entryIdsTagNames = $this->tag_dao->getEntryIdsTagNames($view->entriesId);
+		// The following is a streamable query, i.e. must be last


I kept this comment line, but I didn't understand its meaning

It could be reworded :-P
We cannot have two streamed SQL requests on the same DB connection at the same time

No it's fine, it's just that I wasn't able to spot the difference between this method and the others. But I just realize that fetchAll() isn't called, and I didn't know that PDOStatement implemented the Traversable interface

marienfressinaud · 2020-06-09T17:39:12Z

app/views/importExport/export.phtml

@@ -0,0 +1 @@
+<?= $this->content ?>


This is the real fix of #3035 :)

marienfressinaud · 2020-06-10T08:24:53Z

app/Controllers/importExportController.php

-				Minz_Request::param('export_starred', false),
-				Minz_Request::param('export_labelled', false),
-				Minz_Request::param('export_feeds', array())
+			return Minz_Request::forward(


I just remembered I forgot to explain these return. It's actually useless because the forward(..., true) will immediately trigger a redirection, but it's implicit and not clear for a new developer. I suggest we start to put explicit return when we want the method to stop, what do you think?

Yes, that sounds good indeed

Alkarex · 2020-06-13T10:36:22Z

Looks good.
Not necessarily for this PR, but:

Maybe we should have a default limit for the number of exported articles via the CLI when using the ZIP method (which is currently loading everything in memory and therefore only works for tiny cases):

docker exec --user www-data freshrss cli/export-zip-for-user.php --user alex
FreshRSS exporting ZIP for user “alex”…
PHP Fatal error:  Allowed memory size of 134217728 bytes exhausted (tried to allocate 290816 bytes) in /var/www/FreshRSS/app/Services/ExportService.php on line 175

As it is (also) meant for other tools than FreshRSS, we should apply trim(..., '; ') in

FreshRSS/app/views/helpers/export/articles.phtml

Line 39 in 7a748e2

'author' => $entry->authors(true),

marienfressinaud · 2020-06-13T12:33:35Z

Maybe we should have a default limit for the number of exported articles via the CLI when using the ZIP method

Do you mean the $number_entries variable (default is 100) or something else?

Alkarex · 2020-06-13T12:42:31Z

Yes @marienfressinaud , this is what I mean. I did not check and - due to the memory problem I observed - assumed there was no default limit through the CLI. Now that we have the SQLite export (which uses streams), the ZIP export is less relevant, but I do not like the fact that it uses so much memory and crashes even for light exports. This might be out of scope for this PR though

Alkarex · 2020-06-13T12:53:22Z

Suggestion for future work:

Keep the OPML export as it is;
Keep the JSON export of favourites / labels almost as it is, but modify it to stream the output;
Promote the SQLite export/import so that is is accessible from the Web interface (in addition of CLI);
Remove the ZIP export (but keep the ZIP import for now) as it is a bit redundant and would require more work to re-implement in a memory friendly way.

marienfressinaud commented Jun 9, 2020

View reviewed changes

marienfressinaud commented Jun 10, 2020

View reviewed changes

Alkarex added this to the 1.17.0 milestone Jun 10, 2020

Alkarex approved these changes Jun 13, 2020

View reviewed changes

Alkarex merged commit 15505a0 into FreshRSS:master Jun 13, 2020

marienfressinaud deleted the fix/3035/warning-on-opml-export branch June 14, 2020 07:54

Alkarex modified the milestones: 1.17.0, 1.16.3 Aug 29, 2020

Alkarex mentioned this pull request Sep 17, 2020

Release candidate FreshRSS 1.17.0 #3181

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tec: Refactor the export feature #3045

tec: Refactor the export feature #3045

marienfressinaud commented Jun 9, 2020

marienfressinaud Jun 9, 2020

marienfressinaud Jun 9, 2020

marienfressinaud Jun 9, 2020

Alkarex Jun 10, 2020

marienfressinaud Jun 12, 2020

marienfressinaud Jun 9, 2020

marienfressinaud Jun 10, 2020

Alkarex Jun 10, 2020

Alkarex commented Jun 13, 2020 •

edited

Loading

marienfressinaud commented Jun 13, 2020

Alkarex commented Jun 13, 2020

Alkarex commented Jun 13, 2020 •

edited

Loading

		@@ -720,76 +720,6 @@ private function addFeedJson($origin) {
		return $return;

tec: Refactor the export feature #3045

tec: Refactor the export feature #3045

Conversation

marienfressinaud commented Jun 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alkarex commented Jun 13, 2020 • edited Loading

marienfressinaud commented Jun 13, 2020

Alkarex commented Jun 13, 2020

Alkarex commented Jun 13, 2020 • edited Loading

Alkarex commented Jun 13, 2020 •

edited

Loading

Alkarex commented Jun 13, 2020 •

edited

Loading