Optionally detect tar.* as extension instead of * on views file extension field handler

I really appreciate that you already created a patch against 8.x
Beside tar.gz/tar.bz2 i don't know of an example of this usage of two fileendings,
though if this is not a special case we might should try to implement some option for it?

In general the patch both looks fine to be committed.

Here a patch with the option to choose, first on D8 ;-)

The error on the failing test(Drupal\views\Tests\Handler\AreaTest) is too generic (The test did not complete due to a fatal error.) and I have that test running locally with the patch applied with the new current 8.x.

So, let's see what testbot says on a second run.

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/Extension.phpundefined
@@ -19,10 +19,46 @@
+  protected function defineOptions() {

Should have /** {@inheritdoc} */

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/Extension.phpundefined
@@ -19,10 +19,46 @@
+    $options['fileextension_detect_tar'] = array('default' => FALSE, 'bool' => TRUE);

extension_detect_tar feels better to read. Feel free to change it.

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/Extension.phpundefined
@@ -19,10 +19,46 @@
+   * Default options form that provides the label widget that all fields
+   * should have.

Can you explain what you want to tell us with this comment :)

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/Extension.phpundefined
@@ -19,10 +19,46 @@
+  public function buildOptionsForm(&$form, &$form_state) {

Should have inheritdoc as well.

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/Extension.phpundefined
@@ -19,10 +19,46 @@
+      '#title' => t('Detect if tar is part of the extension'),
+      '#description' => t('Detect multiple tar extensions like tar.gz'),

Maybe bojhan can suggest some better strings.

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/Extension.phpundefined
@@ -19,10 +19,46 @@
+        // See if the previous extension is '.tar' and if so, add that, so we
+        // see 'tar.gz' or 'tar.bz2' instead of just 'gz'.

Hei, this explanation should have been the description before.

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/Extension.phpundefined
@@ -19,10 +19,46 @@
+          $extension = $last_part_in_name . '.' . $extension;

Just to make it more clear we could do 'tar.' . $extension.

Thanks for the suggestions, they all makes sense, I'll follow them.

Can you explain what you want to tell us with this comment :)

Sorry, I based my code on other handlers, that's a docblock I should had removed :-p

Also, as talked on irc, this needs also a test.

Here the suggested changes are incorporated.

The test is not yet working. I am not sure about what I should be trying on the test.
I opted to extend ViewUnitTestBase and re-use views_test_data module test_view view, but it seems like I am overriding the values in the wrong way, so I always get a 'Missing handler: views_test_data extension field'.

Maybe I should just do a full (not unit test) test and create files with several names instead?

Suggestions welcome :-)

You'll need this in the test:
public static $modules = array('file');

@tim.plunkett: Yes, I I'll need that, but probably with more modules, i.e. all the ones mentioned on the base test class ViewUnitTestBase, but that does not really solve my question: should I be doing what I am trying in the patch or make a "full" test uploading files and not using ViewUnitTestBase.

No, $modules is additive, you only need to tack onto whatever is on the parent class.

Several of the more complex handlers have unit and web tests. Case by case basis.

No, $modules is additive, you only need to tack onto whatever is on the parent class.

That's a clever hack I was not aware of :-p, sadly not really intuitive(thinking on simple OOP), thanks for pointing it out!

Several of the more complex handlers have unit and web tests. Case by case basis.

Now that test are running locally, I guess a unit test is enough, since there is not something special to test via web test.

Thanks for writing a test!

+++ b/core/modules/file/lib/Drupal/file/Tests/ExtensionViewsFieldTest.phpundefined
@@ -0,0 +1,81 @@
+  function dataSet() {
...
+  function viewsData() {

doc + public|protected

+++ b/core/modules/file/lib/Drupal/file/Tests/ExtensionViewsFieldTest.phpundefined
@@ -0,0 +1,81 @@
+  public function testFieldExtension() {

needs a small docblock.

+++ b/core/modules/file/lib/Drupal/file/Tests/ExtensionViewsFieldTest.phpundefined
@@ -0,0 +1,81 @@
+
+    $view->displayHandlers->get('default')->overrideOption('fields', array(
+      'name' => array(
+        'id' => 'extension',
+        'table' => 'views_test_data',
+        'field' => 'name',
+      ),
+    ));

I'm not sure, but I think we should better provide a separated test view?

I added changes about method visibility and documentation. Notice I also changed the test method name so the target is more clear.

I'm not sure, but I think we should better provide a separated test view?

I do not think that is really necessary, since we are only trying to test an option on the field handler behavior. BTW I based my test on another test trying to do that, FieldFileSizeTest, which uses that technique. If I am missing something in my reasoning, please let me know.

Well yeah, you probably looked at one of the few examples which manipulate the existing test view instead of creating a new one.

By looking at the yml code you should be able to figure out how to change an existing one, so it's part of the testing.

What's new:

• Some extra documentation changes suggested by Daniel.
• Moves the default view for testing to its own module instead of override it.
• Moves test class inside a Views directory/namespace

What's new: Several extra documentation suggestions made by Daniel.

Thank you for all the effort

Agreed, the patch is looking good.

Committed e8c6f84 and pushed to 8.x. Thanks!

Thanks for all the suggestions and support here!

I'm moving the issue back to D7 views, to actually get the functionality where it was intended originally.

BTW I opened a related issue on #2000058: Document how to make views unit tests.

Here the D7 version, test included :-)

Also, I noticed a minor bug in the added code(wrong default value for form option), so adding also a follow-up patch for D8.

Well, lets do the d8 patch on comment 26 first, so we can get attention on a bug first instead.

I guess we then needs tests for it?

@dawehner: IMHO no, because that's only a miss-use of the form api on a configuration setting we are declaring.

We don't test this kind of behavior anywhere else so maybe just get this in.

Here's the patch from #26 re-uploaded with a name that the testbot will try to test. Agreed on the RTBC, and that this doesn't need tests, but I figured the core committers would appreciate seeing that no other tests break with this applied.

Committed 9b9636c and pushed to 8.x. Thanks!

Thanks for adding it to D8!

Now let's review the patch for D7, it is on comment 26 ;-)

Fixed a trivial indentation bug in ViewsHandlerTestFileExtension::testFileExtensionTarOption() from #26. Haven't actually tested this on D7 yet, but the code looks fine to me.

+++ b/tests/handlers/views_handler_field_file_extension.testundefined
@@ -0,0 +1,66 @@
+class ViewsHandlerTestFileExtension extends ViewsSqlTest {

I think we should suffix this with 'Test', instead of having it in the middle of the class name.

+++ b/tests/handlers/views_handler_field_file_extension.testundefined
@@ -0,0 +1,66 @@
+      'description' => 'Test the core views_handler_field_file_extension handler.',

core seems like Drupal core, and in D7, that's not what this is :) Can we just go with 'Test the views_handler_field_file_extension handler'

+++ b/tests/handlers/views_handler_field_file_extension.testundefined
@@ -0,0 +1,66 @@
+  function dataSet() {
...
+  function viewsData() {

We could do with some docblocks here.

+++ b/tests/handlers/views_handler_field_file_extension.testundefined
@@ -0,0 +1,66 @@
+   * Tests file extension views field handler extension_detect_tar option.

I don't think we need to say it's that handler again - we can just say 'Tests the extension_detect_tar option' or something.

Otherwise, this is looking pretty good!

Actually, every other test class follows the pattern ViewsHandler[Foo]Test. So if anything, we should do that to be consistent (i.e. "Test" as suffix, not prefix).

Otherwise, sure. I'm not above giving out pedantic reviews, so it's only fair I should be subjected to some, myself. ;)

Fixed everything from #35 except the docblocks for dataSet() and viewsData() since those are functions from the parent class and no other views tests document them. If anything, they should use @inheritdoc but so far none of the D7 views tests do that, so I don't want to introduce an inconsistency here.

Didn't I say have test as suffix?! I'm confused. :-)

Sorry, been doing D8 views for too long... so I'm afraid you get a pedantic review including doc blocks.

Whoops, yeah you said suffix. Sorry, I misread. ;)

So, RTBC?

If someone wants to fix all the nitpicks you could theoretically do, feel free to do it. Otherwise I will just get it in.

IMHO I already fixed all the nitpicks, other than introducing some PHPDoc inconsistencies relative to the rest of D7 views in the name of D8 pedantic standards.

I didn't say adhere to all those standards, I just said I've been working in d8 for a long time.

Having the few comments make sense seems valid to me. Especially as they have already been fixed.

For the record, I'm in favor of "pedantic" standards. I'm not objecting to that. But consistency is also important, and I don't want to do things in the D7 code base that we're not doing anywhere else. ;) But whatever, it's a moot point. We're not disagreeing.

Instead, can someone who didn't author this patch please RTBC it?

Thanks,
-Derek

Weird, I didn't touch the tag. Stupid evil comment_alter_taxonomy. :/

The patch is looking good to me.

Thank you very much. Let's get this in for d.o

Committed and pushed to 7.x-3.x

Thanks to all the people pushing this ticket ahead :-)