I'm working on the D7 version of project_release module with the goal of having less custom stuff. This is about the download table on pages like http://drupal.org/project/bad_judgement, the "tar.gz (7.28 KB) | zip (8.47 KB)" part.

In D6, these are tokens provided by project release:

<?php
    $file_parts
= explode('.', basename($item['filename']));
   
$ext = array_pop($file_parts);
   
// See if the previous extension is '.tar' and if so, add that, so we see
    // 'tar.gz' or 'tar.bz2' instead of just 'gz'.
   
$ext2 = array_pop($file_parts);
    if (
$ext2 == 'tar') {
     
$ext = $ext2 . '.' . $ext;
    }
   
$tokens['[' . $this->options['id'] . '-ext' . ']'] = $ext;
?>

In D7, I'm using the handy built-in "File: Extension" field. But it doesn't special-case 'tar'. It would be nice if it did.

Files: 
CommentFileSizeAuthor
#37 1791970-37.views-tar-extenstion-check.patch4.73 KBdww
PASSED: [[SimpleTest]]: [MySQL] 1,636 pass(es).
[ View ]
#37 1791970-37.views-tar-extenstion-check.interdiff.txt1.17 KBdww
#34 1791970-34.views-tar-extenstion-check.patch4.75 KBdww
PASSED: [[SimpleTest]]: [MySQL] 1,636 pass(es).
[ View ]
#31 1791970-31.followup-fix.patch1.2 KBdww
PASSED: [[SimpleTest]]: [MySQL] 56,049 pass(es).
[ View ]
#26 0001-Issue-1791970-by-marvil07-drumm-Added-Optionally-det.patch5.39 KBmarvil07
PASSED: [[SimpleTest]]: [MySQL] 1,636 pass(es).
[ View ]
#26 D8-0001-Issue-1791970-follow-up-do-not-test.patch1.2 KBmarvil07
#21 1791970-21.patch7.06 KBmarvil07
PASSED: [[SimpleTest]]: [MySQL] 56,022 pass(es).
[ View ]
#21 interdiff.txt1.82 KBmarvil07
#20 1791970-20.patch7 KBmarvil07
PASSED: [[SimpleTest]]: [MySQL] 55,911 pass(es).
[ View ]
#18 1791970-18.patch4.7 KBmarvil07
PASSED: [[SimpleTest]]: [MySQL] 54,754 pass(es).
[ View ]
#18 interdiff.txt1.22 KBmarvil07
#16 1791970-16.patch4.52 KBmarvil07
PASSED: [[SimpleTest]]: [MySQL] 54,493 pass(es).
[ View ]
#16 interdiff.txt1.17 KBmarvil07
#11 1791970-11.patch4.66 KBmarvil07
FAILED: [[SimpleTest]]: [MySQL] 54,500 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#11 interdiff.txt5.21 KBmarvil07
#8 0001-Issue-1791970-by-drumm-marvil07-Add-an-option-to-det.patch2.5 KBmarvil07
PASSED: [[SimpleTest]]: [MySQL] 54,539 pass(es).
[ View ]
#6 0001-Issue-1791970-by-drumm-marvil07-Add-an-option-to-det.patch2.5 KBmarvil07
FAILED: [[SimpleTest]]: [MySQL] 54,294 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#2 1791970-d8.diff1.35 KBdrumm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1791970-d8.diff. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 1791970.diff1.36 KBdrumm
PASSED: [[SimpleTest]]: [MySQL] 1,603 pass(es).
[ View ]

Comments

drumm’s picture

Status:Active» Needs review
Issue tags:+project, +Drupal.org D7
StatusFileSize
new1.36 KB
PASSED: [[SimpleTest]]: [MySQL] 1,603 pass(es).
[ View ]

Here is a patch and some tags.

drumm’s picture

StatusFileSize
new1.35 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1791970-d8.diff. Unable to apply patch. See the log in the details link for more information.
[ View ]

And here is a completely-untested D8 version.

Status:Needs review» Needs work

The last submitted patch, 1791970-d8.diff, failed testing.

drumm’s picture

Status:Needs work» Needs review

I still don't know the magic filenames or whatever that testbot wants.

dawehner’s picture

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.

marvil07’s picture

Title:Show tar.gz as extension instead of gz» Optionally detect tar.* as extension instead of * on views file extension field handler
Project:Views» Drupal core
Version:7.x-3.x-dev» 8.x-dev
Component:files/upload data» views.module
Assigned:drumm» marvil07
Issue tags:+needs backport to D7
StatusFileSize
new2.5 KB
FAILED: [[SimpleTest]]: [MySQL] 54,294 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 0001-Issue-1791970-by-drumm-marvil07-Add-an-option-to-det.patch, failed testing.

marvil07’s picture

Status:Needs work» Needs review
StatusFileSize
new2.5 KB
PASSED: [[SimpleTest]]: [MySQL] 54,539 pass(es).
[ View ]

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.

dawehner’s picture

+++ 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.

marvil07’s picture

Status:Needs review» Needs work

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.

marvil07’s picture

Status:Needs work» Needs review
StatusFileSize
new5.21 KB
new4.66 KB
FAILED: [[SimpleTest]]: [MySQL] 54,500 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

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 :-)

tim.plunkett’s picture

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

Status:Needs review» Needs work

The last submitted patch, 1791970-11.patch, failed testing.

marvil07’s picture

@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.

tim.plunkett’s picture

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.

marvil07’s picture

Status:Needs work» Needs review
StatusFileSize
new1.17 KB
new4.52 KB
PASSED: [[SimpleTest]]: [MySQL] 54,493 pass(es).
[ View ]

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.

dawehner’s picture

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?

marvil07’s picture

StatusFileSize
new1.22 KB
new4.7 KB
PASSED: [[SimpleTest]]: [MySQL] 54,754 pass(es).
[ 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.

dawehner’s picture

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.

marvil07’s picture

StatusFileSize
new7 KB
PASSED: [[SimpleTest]]: [MySQL] 55,911 pass(es).
[ View ]

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
marvil07’s picture

StatusFileSize
new1.82 KB
new7.06 KB
PASSED: [[SimpleTest]]: [MySQL] 56,022 pass(es).
[ View ]

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

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Thank you for all the effort

damiankloip’s picture

Agreed, the patch is looking good.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed e8c6f84 and pushed to 8.x. Thanks!

marvil07’s picture

Project:Drupal core» Views
Version:8.x-dev» 7.x-3.x-dev
Component:views.module» files/upload data
Status:Fixed» Needs work

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.

marvil07’s picture

Status:Needs work» Needs review
Issue tags:-needs backport to D7
StatusFileSize
new1.2 KB
new5.39 KB
PASSED: [[SimpleTest]]: [MySQL] 1,636 pass(es).
[ View ]

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.

marvil07’s picture

Project:Views» Drupal core
Version:7.x-3.x-dev» 8.x-dev
Component:files/upload data» views.module
Category:feature» bug

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

dawehner’s picture

I guess we then needs tests for it?

marvil07’s picture

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

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

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

dww’s picture

StatusFileSize
new1.2 KB
PASSED: [[SimpleTest]]: [MySQL] 56,049 pass(es).
[ View ]

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.

alexpott’s picture

Project:Drupal core» Views
Version:8.x-dev» 7.x-3.x-dev
Component:views.module» files/upload data
Status:Reviewed & tested by the community» Patch (to be ported)

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

marvil07’s picture

Status:Patch (to be ported)» Needs review

Thanks for adding it to D8!

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

dww’s picture

StatusFileSize
new4.75 KB
PASSED: [[SimpleTest]]: [MySQL] 1,636 pass(es).
[ View ]

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.

damiankloip’s picture

+++ 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!

dww’s picture

Status:Needs review» Needs work

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. ;)

dww’s picture

Status:Needs work» Needs review
StatusFileSize
new1.17 KB
new4.73 KB
PASSED: [[SimpleTest]]: [MySQL] 1,636 pass(es).
[ View ]

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.

damiankloip’s picture

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.

dww’s picture

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

So, RTBC?

dawehner’s picture

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

dww’s picture

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.

damiankloip’s picture

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.

dww’s picture

Issue tags:-project

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

dww’s picture

Issue tags:+project

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

damiankloip’s picture

Status:Needs review» Reviewed & tested by the community

The patch is looking good to me.

dawehner’s picture

Status:Reviewed & tested by the community» Fixed

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

Committed and pushed to 7.x-3.x

marvil07’s picture

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

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