Don't want to step on any toes, but the way views support was implemented is incorrect...

For example:

 $data['stormtask']['organization_nid'] = array(
	'title' => t('Task Organization Node ID'),
 	'help' => 'Storm Task Organization Node ID',
 	'field' => array(
        	'click sortable' => TRUE,
 	),
 	'sort' => array(
 	        'handler' => 'views_handler_sort',
 	),
 	'filter' => array(
 	        'handler' => 'views_handler_filter_numeric',
 	),
 	'argument' => array(
 	        'handler' => 'views_handler_argument_numeric',
 	),
);

should have been done the method shown in the example from hook_views_data:

// Node ID field.
  $data['example_table']['nid'] = array(
    'title' => t('Example content'),
    'help' => t('Some example content that references a node.'),
    // Because this is a foreign key to the {node} table. This allows us to
    // have, when the view is configured with this relationship, all the fields
    // for the related node available.
    'relationship' => array(
      'base' => 'node',
      'field' => 'nid',
      'handler' => 'views_handler_relationship',
      'label' => t('Example node'),
    ),
  );

By defining the nids as numerics, rather than related node ids... you've completely neutered any ability to make a complex relationship using view, because views doesn't understand that the nid field is actually an nid, it thinks it's only some arbitrary number. By using the proper views handler (views_handler_relationship), views will let you do great things, like putting info from the parent projects when viewing a list of tasks, viewing fields of an org when viewing the items in that org, etc...

Looks like I need to build this out correctly, since I've got a request for some complex views using Storm as a base.
Has anyone else done this, and not submitted it yet (a search in the issue queue for 'views' didn't find anything)?

If not, I'll try and get a patch submitted shortly, after it's built.

CommentFileSizeAuthor
#7 storm.patch55.5 KBsethcohn
#4 storm-D6.patch55.5 KBsethcohn
#2 storm.patch57.37 KBsethcohn

Comments

sethcohn’s picture

sethcohn commented

working on this... good progress. Also cleaning up the missing t functions, and relabeling items/groups to improve the views interface clutter.

sethcohn’s picture

sethcohn commented
Version: 6.x-1.x-dev » 6.x-1.33
Component: Miscellaneous » Storm.module
Status: Active » Needs review
StatusFileSize
new storm.patch57.37 KB

patch attached

Please review

Status: Needs review » Needs work

The last submitted patch, storm.patch, failed testing.

sethcohn’s picture

sethcohn commented
StatusFileSize
new storm-D6.patch55.5 KB

oops.

sethcohn’s picture

sethcohn commented
Status: Needs work » Needs review
juliangb’s picture

juliangb commented
Version: 6.x-1.33 » 6.x-1.x-dev

Needs to patch based on -dev. Also, patch will not be tested with the D6 ending... needs reuploading without.

sethcohn’s picture

sethcohn commented
StatusFileSize
new storm.patch55.5 KB

Patch applied to dev just fine...

renamed to remove the D6...

Status: Needs review » Needs work

The last submitted patch, storm.patch, failed testing.

sethcohn’s picture

sethcohn commented
Status: Needs work » Needs review

The exceptions are in the existing module code, not my patch, but it still fails. That is quite annoying.

tchurch’s picture

tchurch commented

Tell me about it. I have patches on hold because of it.

I'm working on sorting this out.

#684016: Fix E_NOTICE notifications

Hopefully soon.

juliangb’s picture

juliangb commented

We´ll need to retest after #684016: Fix E_NOTICE notifications - please help out on that issue to speed it along if you can.

What is the impact of this patch on existing views? Will anything break?

This is an area I do not know about that much, so hopefully it would be possible for a couple of people to review.

sethcohn’s picture

sethcohn commented

Potentially, yes, some views will break... the handlers have changed... it's really not a good idea to try and leave reverse compatibility though, it'll be quite confusing. The improvements are substantial, and IMHO, worth the impact.

dbt102’s picture

dbt102 commented

I have many Storm-Views-CCK screens setup at this point and a small group of users are dependent on the system. But I (we) would MUCH prefer to have Views correctly implemented in Storm, even if it means I have to revisit my Views and set them up again, they will only get better next time around :-)

I revisited the closed discussion at http://drupal.org/node/293485, where a little over a year ago prevailing sentiment was NO dependencies, even for CCK and Views. I'm glad Storm got through that, and CCK-Views has made it an even more valuable tool (along with an assortment of other great modules of course :.)

D7 is looking really good with each new alpha release. Seems like getting Views implemented correctly NOW would ease the migration to D7 platform. Also, it looks like Panels is moving forward quickly. Does something need built-out in Storm to hook into Panels functionality?

sethcohn’s picture

sethcohn commented

#7: storm.patch queued for re-testing.

sethcohn’s picture

sethcohn commented

Ok, patch passed, thanks to tchurch fixing the exceptions problem.

juliangb’s picture

juliangb commented

OK, would be good to have some reviews - db102, perhaps you could if you have several views set up already?

juliangb’s picture

juliangb commented

#7: storm.patch queued for re-testing.

sethcohn’s picture

sethcohn commented

Patch still passes, and doesn't look like enough folks are using views to do much testing (which is a poor sign of how bad views support was before, perhaps?)

I'd like to see this called RTBC if someone would please try it out and comment..

juliangb’s picture

juliangb commented

Its more a sign that people don't like testing patches i'm afraid.

I'll get to it, not sure when though.

juliangb’s picture

juliangb commented 
+++ stormexpense/stormexpense.views.inc	Mon Jan 19 17:26:36 1970
@@ -165,25 +141,19 @@
-    'field' => array(
-      'click sortable' => TRUE,
-    ),
-    'sort' => array(
-      'handler' => 'views_handler_sort',
-    ),
-    'filter' => array(
-      'handler' => 'views_handler_filter_numeric',
-    ),
-    'argument' => array(
-      'handler' => 'views_handler_argument_numeric',

The patch removes a lot of these types of arrays without replacing them.

Do I understand therefore that they are not necessary?

Powered by Dreditor.

tchurch’s picture

tchurch commented

Sorry, trying to catch up on emails after a holiday. I also have customers that use Storm and I try and also use views where I can. I must admit I would use views more with my customers if more fields and filters were defined but I'm not really familiar with views integration.
I can try and test patch when it's OK.

Wappie08’s picture

Wappie08 commented

Hello, thanks for the patch!

I think the (better) implementation with views is very important, lots of ppl are using views and are trying to make custom views. If you configure some displays for clients for example, you have to use views. Also I think the use of Storm will go up if there will be better intgration and default/example-views with the module in the future.

I would like to test the patch if I can find time, what would you guys want me to test? I just installed storm and created a couple views-blocks and put them in panel-pages. All works quite well without patch..

Greetings Bas

juliangb’s picture

juliangb commented

The way to test this - basically to apply the patch and use the patched version. If anything seems to have broken, then report it here.

Also, it would be worth testing the affect on existing views simply so that we can provide good support if things do break.

I'm still a little unsure about how this patch changes the implementation, so would also like a little more explanation based on #20.

That said though, I'd really like to get this in for the next release if the patch is good - as the views implementation will be more and more important.

sethcohn’s picture

sethcohn commented

Answer to #20 and #23 (sorry, been focusing on other things in reallife, Drupal work has been out of my loop):

Yes, it removes all unneeded arrays, which were part of the problem. It treated nids as numerics, rather than the correct way (as nids, which Drupal knows what to do with it, if it's told that what something is)

As stated, _some_ existing views are going to break. Plain and simple: they were done SO wrong that old views were useless in lots of ways. I did leave as much as possible intact, but leaving too much merely keeps legacy views working at a cost of having confusing new view choices that shouldn't exist. If anything, I've left too much of the old, but that's more a function of the way Storm ends up duplicating data it shouldn't have (true node reference usage would solve that, which is beyond the scope here).

If someone is a view savvy person, they will love the new views support. The only folks with an issue are those who built views using the existing limited support, and I think the benefits will outweigh any adjustments they need to make for the changes. But I wonder how many there are... The poor views support seems to have made it so few folks are using it now anyway....

juliangb’s picture

juliangb commented

#7: storm.patch queued for re-testing.

juliangb’s picture

juliangb commented
Status: Needs review » Fixed

I've committed this patch.

I'm not greatly knowledgeable about how the views support should be implemented, but @sethcohn has answered all of my queries about it.

I will add a note to the changelog that custom views may have to be rebuilt.

Status: Fixed » Closed (fixed)

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