[RFC] CSV import for options form field #444

qzminski · 2016-02-12T10:03:18Z

Added the CSV import feature for the options field in the form generator. Also introduced the CSV Import utility which is now used also by ListWizard and TableWizard widgets.

Toflar · 2016-02-15T10:09:01Z

Thank you for the PR Kamil! I would like to make some general comments on what I don't like about the architecture of this PR. I think, if you added the unit tests - that are mandatory to make it into the core-bundle - already, you would have noticed some of the problems :-)

General question: I think there are many CSV libraries out there and I wonder why we don't make use of one of them?
There are a few violations of separation of concerns. Especially, the architecture of CsvImportUtil feels very strange to me:

a) Why would it need the framework? Contao\Message should be adapted so it can handle Symfony's flash messages, which would immediately drop the need of that one.
b) Why would it need the framework part 2: You don't need System::setCookie(). Firstly, we set cookies on the request now, not on System anymore and then why is it the task of the CsvImportUtil to set cookies?
c) Why does the CsvImportUtil throw RedirectResponseException's? Redirecting is certainly not a task of a "csv import util" but belongs to a controller :-)

A good indicator of wrong dependencies are the constructor arguments. Why should a csv import utility care about a service named tokenStorage?

Imho, you have a good start here but what you need to do is to split up the tasks of your CsvImportUtil even more:

The real CSV handling stuff, I doubt you need to reinvent the wheel here and you can maybe use a third party library from packagist.
The view component which is responsible to generate your back end views.

qzminski · 2016-02-15T10:36:45Z

Just as a word for introduction, the class is currently designed to handle only the backend CSV import that is commonly used for different widgets in Contao.

Why would you like to use the CSV library if reading a CSV file is a matter of three lines of code using fgetcsv function?

2a. Indeed Contao\Message should be adapted to Symfony's flash messages but at the time being it is not. Once it is adapted, I agree it should be used instead.

2b. That was just copy-pasted from the old method. Can go the controller.

2c. Yeah, that should probably go to the controller.

The tokenStorage is passed to the class so it can determine the current user file uploader. It can belong to controller as well which will tell which FileUpload instance should the class use.

I'll introduce those changes soon. Controller will grow a little bigger than it currently is but that shouldn't be a problem huh? @aschempp ?

aschempp · 2016-02-15T11:50:26Z

I haven't completely reviewed the code yet, but I remember we discussed the FileUpload instance should be passed to the utility class, which would probably make $tokenStorage unnecessary.

aschempp · 2016-02-15T11:51:35Z

src/Controller/BackendCsvImportController.php

+
+        $csvImport = new CsvImportUtil(
+            $this->connection,
+            new Message(),


Should use $this->framework->createInstance('Contao\Message');

Of course not.

All the methods are static??

Ah well, so you mean we should use getAdapter :D

Toflar · 2016-02-15T11:57:50Z

2a. Indeed Contao\Message should be adapted to Symfony's flash messages but at the time being it is not. Once it is adapted, I agree it should be used instead.

Here you go: #445 😄

qzminski · 2016-02-15T12:50:56Z

I have introduced some improvements. However, I am not sure how to move RedirectResponseException to the controller. Should then controller listen on util class if it throws a regular \Exception and if so trigger RedirectResponseException? Any suggestions?

aschempp · 2016-02-15T14:11:07Z

I have introduced some improvements. However, I am not sure how to move RedirectResponseException to the controller. Should then controller listen on util class if it throws a regular \Exception and if so trigger RedirectResponseException? Any suggestions?

I'm not entirely sure. But maybe it should throw an "generic" exception including the error message that is catched by the controller. The controller would then add the flash message, and it would do the reload. For the other redirect, the run() method could return true/false (or just nothing, really) and the controller could then also do a redirect. It always needs to redirect if run() succeeds (which feels kinda strange…)

qzminski · 2016-02-17T09:31:01Z

I have done some improvements, please review them.

discordier · 2016-02-17T10:09:34Z

src/Controller/BackendCsvImportController.php

+            }
+
+            // Set the backend offset cookie
+            $this->framework->getAdapter('Contao\System')->setCookie('BE_PAGE_OFFSET', 0, 0);


I wonder if we really have to reset the Cookie.
If not, we only need to inject the max filesize below and are completely independent from the framework in this class, which I consider a goal for every new class in Contao.

I guess that's just how it currently behaves…

Can't we reset the cookie in other way, i.e. not through Contao framework? Create the RedirectResponse with appropriate cookie which would then be used as constructor argument in ResponseException?

Pretty sure you can (and should!) add the cookie to the response :-)

aschempp · 2016-02-17T10:34:11Z

I think we should fall back to FileUpload if no token or user is available. Also, it should be possible to set the uploader script.

qzminski · 2016-02-17T10:53:22Z

@aschempp uploader fallback set in the CsvImportUtil itself or in the backend controller? Frankly I would put it in the utility class so it behaves just similar to the getTemplate() method (returns fallback if no template was set).

aschempp · 2016-02-17T11:00:47Z

Sorry, I totally agree. But the controller should fall back to the default if the user can't be found (instead of not creating an instance of the utility).

aschempp · 2016-03-03T07:48:19Z

I think this can be updated to RFC or RTM, what do you think?

qzminski · 2016-03-03T08:05:29Z

@contao/developers - request for feedback

leofeyer · 2016-03-03T08:14:22Z

RTM = ready to merge
RFC = request for comments

I have changed the title accordingly :)

aschempp · 2016-03-31T11:38:17Z

bump

Toflar · 2016-04-28T15:05:45Z

src/Controller/BackendCsvImportController.php

+ *
+ * @author Kamil Kuzminski <https://github.com/qzminski>
+ */
+class BackendCsvImportController


Controllers in Symfony use Action as method suffix. I think they should all use that too here.

Toflar · 2016-04-28T15:18:57Z

As I stated on February 15th already, Unit Tests are missing. I know that not everything of it will be testable. Especially the parts where we are working in legacy code. However, the controllers as well as the import util is new code and if it's not unit testable, something must be wrong. We need tests here :)

qzminski · 2016-04-28T17:21:38Z

@Toflar can you please review if the last commits cover our Skype discussion conclusions?

Toflar · 2016-04-28T18:57:08Z

src/Resources/config/services.yml

+    contao.controller.abstract_csv_import:
+        class: Contao\CoreBundle\Controller\AbstractCsvImportController
+        abstract: true
+        calls:


As these are mandatory, you must use constructor injection rather than setter injections. Setter injections are for optional dependencies.

I have changed that but left the setters to allow service overriding by child classes.

leofeyer · 2017-01-24T14:09:02Z

src/Controller/BackendCsvImportController.php

+     *
+     * @return Response
+     */
+    protected function importFromTemplate(


CS violation

leofeyer · 2017-01-24T14:10:25Z

src/Controller/BackendCsvImportController.php

+    private function getSeparators($allowLinebreak = false)
+    {
+        $separators = [
+            self::SEPARATOR_COMMA     => [


CS violation

leofeyer · 2017-01-24T14:13:39Z

src/Controller/BackendCsvImportController.php

+                throw new \RuntimeException(sprintf($GLOBALS['TL_LANG']['ERR']['filetype'], $extension));
+            }
+
+            // Drop TL_ROOT when we got rid of FileUpload which uses it


The comment can be removed. It is missing the TODO: prefix and I guess we are all aware of the TL_ROOT deprecation.

leofeyer · 2017-01-24T14:17:49Z

I actually don't understand why we need this change. Now we have rewritten two of many legacy widgets – why not all of them? We have had the exact same discussion in #355 and this was the summary:

As discussed in Mumble on April 28th, this change is a mixture between legacy code and new code, which we cannot really befriend with.

What makes this PR different?

leofeyer · 2017-01-27T10:29:49Z

@qzminski Can you please give me push access for this PR?

https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

qzminski · 2017-01-27T10:33:14Z

Just did.

leofeyer · 2017-01-27T10:39:10Z

Thanks. 😉

leofeyer · 2017-01-27T10:47:58Z

I have replaced

<?php if ($this->messages): ?>
    <div class="tl_message">
        <?php foreach ($this->messages as $type => $messages): ?>
            <?php foreach ($messages as $message): ?>
                <p class="tl_<?= $type ?>"><?= $message ?></p>
            <?php endforeach; ?>
        <?php endforeach; ?>
    </div>
<?php endif; ?>

with

<?= Message::generate() ?>

which does exactly the same, doesn't it? Or am I missing something?

leofeyer · 2017-01-27T10:53:17Z

src/Controller/BackendCsvImportController.php

+    {
+        $this->framework->initialize();
+
+        $request = $this->requestStack->getCurrentRequest();


$request can be null and we are not handling the case. What would be the right thing to do in this case?

throw an exception? We can't handle that case, it cannot happen in the Contao backend.

qzminski · 2017-01-27T11:07:37Z

I think the only difference with Message::generate() is that it gets the flash messages from the session top level service, whereas the initial code takes those messages from the session bound to the request object we use in controller. But I guess that session instance is always the same.

Also previously we fetched the messages by getting all() of them from flash bag whereas Message takes only those of Contao scope. But that should not be an issue here either.

Thus I'm fine with that change, just also drop line 214 in that case:

https://github.com/qzminski/contao-core-bundle/blob/1023d3c798d71bd037dc43e0eb0542f8ef7df03a/src/Controller/BackendCsvImportController.php#L214

aschempp · 2017-01-27T11:13:50Z

Be aware that there is only one session. Subrequests will inherit the main session from the master request, so I guess @leofeyer is right with the changes.

leofeyer · 2017-01-27T11:57:58Z

Big thanks @qzminski, @aschempp and @Toflar. This was a huge PR. 😄

* Make Symfony 3.2 the minimum requirement (see contao#630). * Set the encryption key from the kernel secret by default (see contao#660). * Stop using the deprecated QuestionHelper::setInputStream() method. * Update Dropzone to version 4. * Prefer the caret operator over the tilde operator in the composer.json file. * Add the contao.root_dir parameter (see contao#662). * Update the change log. * Stop using the contao-components/all meta package. * Update the README.md file. * Deprecate the contao:version command (see contao#668). * Update the installation path. * Auto-select the active page in the quick navigation/link module (see contao/core#8587). * Look up the form class and allow to choose the type (see contao/core#8527). * Add PHP Toolbox support (see contao#565). * Remove the arrow brackets in the book navigation template (see contao/core#8607). * Add a bottom padding to the buttons layer. * Add the contao.web_dir parameter (see contao/installation-bundle#40). * Fix the tests. * Match security firewall based on request scope (see contao#677). * Fix an issue found by Scrutinizer. * Use the contao.web_dir parameter in the Combiner (see contao#679). * Fix the tests. * Add stripRootDir() method to System class (see contao#683). * Add the contao.image.target_dir parameter (see contao#684). * The ContaoCoreExtension::overwriteImageTargetDir() is not deprecated. * Support custom backend routes (see contao#512). * Use the scope matcher instead of checking the request attribute (see contao#688). * Replace every occurrence of $contaoFramework with $framework. * Fix an issue found by Scrutinizer. * Fix deprecations in unit tests (see contao#687). * Added a DBAL field type for UUIDs (see contao#415). * Support importing form field options from a CSV field (see contao#444). * Fix the coding style and the unit tests. * Add the Doctrine field type in the config.yml file. doctrine: dbal: types: binary_string: class: "Contao\\CoreBundle\\Doctrine\\DBAL\\Types\\BinaryStringType" commented: true * Add a basic unit test for the BackendCsvImportController class. * Update the change log. * Fix rebuilding the search index (see contao#689). * Also handle „no origin“ and „empty origin“ in the CORS provider. * Remove an unused use statement. * Remove the security.yml file and update the README.md file. * Improve the e-mail extraction in the text element (thanks to Martin Auswöger). * Rename the Test namespace to Tests. * Update the composer.json file. * Update the .php_cs file. * Raise the minimum PHP version to 5.6 (see contao#701). * Support using objects in callback arrays (see contao#699). * Use try-finally blocks to close all output buffers when downloading a file (see contao#714). * Fix the coding style. * Only prefix an all numeric alias when standardizing (see contao#707). * Adjust the test namespaces. * Allow to manually pass a value to any widget (see contao#674). * Add a change log entry and fix the tests. * Disable the picker buttons if the main window does not show a picker. * Use the file manager instead of the file picker. * Use the site structure instead of the page picker. * Always show the selected nodes. * Add the menu builder.

…mplate (see #444)

leofeyer added the feature label Feb 12, 2016

leofeyer added this to the 4.2.0 milestone Feb 12, 2016

aschempp reviewed Feb 15, 2016
View reviewed changes

discordier reviewed Feb 17, 2016
View reviewed changes

qzminski changed the title ~~[POC] CSV import for options form field~~ [RTM] CSV import for options form field Mar 3, 2016

leofeyer changed the title ~~[RTM] CSV import for options form field~~ [RFC] CSV import for options form field Mar 3, 2016

leofeyer modified the milestone: 4.2.0 Apr 15, 2016

Toflar reviewed Apr 28, 2016
View reviewed changes

qzminski and others added 4 commits January 24, 2017 12:06

Fixed the relative file path problem

d8ee54b

Fixed the data handling for table and option wizards

1e5859c

Fixed the template markup

f1ffec4

Updated deprecated notices

7ba5e32

aschempp force-pushed the feature/options-import branch from 97c9267 to 7ba5e32 Compare January 24, 2017 12:00

leofeyer requested changes Jan 24, 2017

View reviewed changes

leofeyer added up for discussion and removed up for discussion labels Jan 24, 2017

leofeyer added this to the 4.4.0 milestone Jan 25, 2017

aschempp added 2 commits January 25, 2017 14:22

Use the contao.root_dir parameter instead of TL_ROOT

3d906a9

CS

6dd9db3

Fix the coding style.

182b63b

leofeyer force-pushed the feature/options-import branch from d96ab0d to 182b63b Compare January 27, 2017 10:46

leofeyer requested changes Jan 27, 2017

View reviewed changes

Add an inline phpDoc comment for the Config adapter.

1023d3c

Throw an exception if there is no request object.

5c15ae4

leofeyer approved these changes Jan 27, 2017

View reviewed changes

leofeyer self-assigned this Jan 27, 2017

leofeyer merged commit 481012f into contao:develop Jan 27, 2017

agoat mentioned this pull request Apr 10, 2017

Feature/dca picker (#46) agoat/contao-core-bundle#47

Merged

leofeyer added a commit that referenced this pull request Apr 25, 2019

Fix the preload attribute for the audio tag in the ce_player.html5 te…

c4ebd01

…mplate (see #444)

[RFC] CSV import for options form field #444

[RFC] CSV import for options form field #444

Conversation

qzminski commented Feb 12, 2016

Toflar commented Feb 15, 2016

qzminski commented Feb 15, 2016

aschempp commented Feb 15, 2016

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

Toflar commented Feb 15, 2016

qzminski commented Feb 15, 2016

aschempp commented Feb 15, 2016

qzminski commented Feb 17, 2016

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

aschempp commented Feb 17, 2016

qzminski commented Feb 17, 2016

aschempp commented Feb 17, 2016

aschempp commented Mar 3, 2016

qzminski commented Mar 3, 2016

leofeyer commented Mar 3, 2016

aschempp commented Mar 31, 2016

Choose a reason for hiding this comment

Toflar commented Apr 28, 2016

qzminski commented Apr 28, 2016

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

leofeyer commented Jan 24, 2017

leofeyer commented Jan 27, 2017

qzminski commented Jan 27, 2017

leofeyer commented Jan 27, 2017

leofeyer commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qzminski commented Jan 27, 2017

aschempp commented Jan 27, 2017

leofeyer commented Jan 27, 2017