Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 379039

Summary: [pmi] Display project specific information for project-related content types
Product: Community Reporter: Wayne Beaton <wayne.beaton>
Component: Project Management & PortalAssignee: Portal Bugzilla Dummy Inbox <portal-inbox>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: nathan
Version: unspecified   
Target Milestone: ---   
Hardware: PC   
OS: Linux   
Whiteboard:
Bug Depends on: 379192    
Bug Blocks:    
Attachments:
Description Flags
Patch for shortened hook name
none
Patch for shortened hook name
none
mylyn/context/zip none

Description Wayne Beaton CLA 2012-05-09 14:34:39 EDT 
There's a couple of places in the code where we need to get the project associated with an arbitrary node. This is probably most useful for project-specific that we want to display when we're navigating through project-specific things. If I'm looking at a release, election, role, etc. for a project, I think it makes sense to show information for the associated project. This is a generalization of Bug 378914.

e.g. _releases_get_project($node) in releases.module, and project_blocks_block_view($delta) in project_blocks.module. There may be others.

This seems like some generally useful functionality that we should probably move into projects.module. At very least, there should be a function so that we don't keep solving the same problem over-and-over.

The implementation in releases.module leverages the objects we associate with content types. This will work for those content types that have a corresponding classes. Project pages do not have a corresponding class, and so will need to be accomodated in another way.

How about something like this:

/**
 * Determine the project that is most appropriate for the node.
 * If the node represents a project, then the corresponding Project
 * instance is returned. Otherwise, we try to sort out the most 
 * appropriate project for the node.
 * 
 * @param StdClass $node A Drupal node
 * @return Project|NULL
 */
function projects_get_related_project($node) {	
	if ($project = Project::getInstance($node)) return $project;
	
	if ($object = commons_get_object($node) && method_exists($object, 'getProject'))
		return call_user_func(array($object, 'getProject'));
		
	$projects = module_invoke_all('related_project');
		// Regardless of what we do module_invoke_all returns an array, 
		// we're always going to take the first one.
		if (count($project_array)) $project = $project_array[0];
	}
	return null;
}

The use of a hook here may be a bit of overkill; we'll wind up calling a lot of different hook implementations. We may be able to do better with a call_user_func

$function = $node->type . '_get_related_project';
if (function_exists($function)) return call_user_func($function, $node)

Then we can implement functions like function page_get_related_project($node) that return exactly one result.

Or something like that.

Thoughts?
Comment 1 Nathan Gervais CLA 2012-05-09 14:56:52 EDT 
(In reply to comment #0)

> The use of a hook here may be a bit of overkill; we'll wind up calling a lot of
> different hook implementations. We may be able to do better with a
> call_user_func
> 
> $function = $node->type . '_get_related_project';
> if (function_exists($function)) return call_user_func($function, $node)
> 
> Then we can implement functions like function page_get_related_project($node)
> that return exactly one result.

I think this is the more elegant solution as it would only call the relevant hook

The minor quirk here is that I would use a page_get_related_project function here instead of a project_pages_get_related_project which might clutters the module namespace a bit but I think its something we can live with.
Comment 2 Wayne Beaton CLA 2012-05-09 14:59:34 EDT 
For completness, once this functionality has been implemented, we should ensure that it works as expected for elections, releases, and projectroles as well as custom pages.
Comment 3 Wayne Beaton CLA 2012-05-09 15:00:18 EDT 
(In reply to comment #1)
> The minor quirk here is that I would use a page_get_related_project function
> here instead of a project_pages_get_related_project which might clutters the
> module namespace a bit but I think its something we can live with.

A more important consideration is that another module may try to define the same function. This would be bad.
Comment 4 Wayne Beaton CLA 2012-05-10 19:48:18 EDT 
I've commited a variation of what's been discussed here.

The projects__get_related_project function first checks to see if the node is of the "project" content type and returns the corresponding Project class instance if it is. Otherwise, if there is a wrapper class that corresponds to the content type that has a getProject method, an instance is created and sent the getProject message. If all else fails, the hook hook_projects_related_project is invoked. If all fails, the function just returns NULL.

I can't fully test the hook functionality until Bug 379192 is addressed.
Comment 5 Nathan Gervais CLA 2012-05-11 00:30:25 EDT 
(In reply to comment #4)
> I've commited a variation of what's been discussed here.
> 
> The projects__get_related_project function first checks to see if the node is
> of the "project" content type and returns the corresponding Project class
> instance if it is. Otherwise, if there is a wrapper class that corresponds to
> the content type that has a getProject method, an instance is created and sent
> the getProject message. If all else fails, the hook
> hook_projects_related_project is invoked. If all fails, the function just
> returns NULL.
> 
> I can't fully test the hook functionality until Bug 379192 is addressed.

After closing out Bug 379188 i noticed that in the project pages version of the hook my function name is 'project_pages_projects_related_project'

There are 3 different mentions of the word project, and the hook itself contains two.  Can we change this to hook_related_project()?
Comment 6 Nathan Gervais CLA 2012-05-11 00:44:03 EDT 
Created attachment 215439 [details]
Patch for shortened hook name

Attached is a patch for shortening the hook name, fixing a refactoring issue with mismatched variable name, and added the hook_related_projects call to the release module.
Comment 7 Nathan Gervais CLA 2012-05-11 00:45:03 EDT 
Created attachment 215440 [details]
Patch for shortened hook name

Same comment as before only this time i'll check the patch button
Comment 8 Wayne Beaton CLA 2012-05-11 07:32:05 EDT 
(In reply to comment #5)
> There are 3 different mentions of the word project, and the hook itself
> contains two.  Can we change this to hook_related_project()?

Pretty crazy. What is the Drupal convention for naming hooks?
Comment 9 Nathan Gervais CLA 2012-05-11 09:38:26 EDT 
After a cursory Google search I found this post in a thread about naming hooks.

http://grokbase.com/t/drupal/development/09afy65wr1/convention-for-naming-contrib-hooks#200910157s6kf9tq1gen8bamzq0mc1b63g

Larry Garfield is a committer on Drupal Core if that lends any credence to his statement.

It would seem your original proposal matches the convention.  Its not pretty to read but i understand its purpose.
Comment 10 Wayne Beaton CLA 2012-05-11 10:03:27 EDT 
Maybe hook_projects_related is enough?

I also provided a stub implementation of the hook; I've seen this other modules. I think it's a good convention to provide a template implementation of every hook as it provides a convenient means of documenting the existence of the hook, providing documentation for its use, and makes content assist possible. Thoughts?
Comment 11 Wayne Beaton CLA 2012-05-11 13:16:01 EDT 
I've switched it to hook_projects_related. Marking as FIXED.
Comment 12 Wayne Beaton CLA 2012-05-11 13:16:03 EDT 
Created attachment 215492 [details]
mylyn/context/zip
Comment 13 Nathan Gervais CLA 2012-05-11 15:49:10 EDT 
I've committed some changes to a few modules that rename any function using the old _module_name_function_call to module_name__function call.