Convert Graph.inc to PSR-0 (and make its documentation clearer)

1) This is a perfect use case for a Component rather than another Core subsystem.

2) Even though there's no persistent data, I think we'd be better off making this an instantiated object. It's just cleaner, and pure-logic objects have nothing wrong with them.

New patch attached. I disagree with needing to instantiate Graph. We really don't need that - it's just a wrapper around a function, and there's no state that we need to keep track of in the class. If someone else wants to add it, that's fine, but I'd rather not.

New new patch attached. Forgot newline at the end of Graph.php. Also, needs review for bot.

If it's static, then you get zero benefit over it being a function except for autoloading. If it's instantiated, then you can inject it and avoid a hard dependency. That improves unit testing, since otherwise it is impossible to test something that uses Graph without also testing Graph.

Let's try this one. Should have less failures.

Will look at the static stuff when the bot comes back.

Now without static stuff...
This means I had to invent a name for the object. Called it graphObject for now but it needs a better name...

And we shouldn't camelcase variables....

Updated patch...

Commence bikeshedding! This is by no means a graph class. It does not have a graph storage, there are no graph manipulation methods, there's absolutely nothing. I have no idea what was your problem with the function but if you are on a conversion spree then at least name the class for what it is: a depth first search class.

Would help a little, yes. Leaving alone the include file would be even better. I fail to see the object here: no properties and thus no methods. It's not pluggable either. Why are we converting this file...? Autoloading? Sure...

Awesome! I started working on a patch for this as well. Would be nice to compare the current patch with where I'm at to see if I'm doing it right/wrong.

#15 makes sense to me.

Renamed everything (I hope). Lets see what bot thinks...

Variables should not use camelCase, see #11 ;)
Other than that I think you renamed it everywhere.

ARGH!

I'll never learn it...

@aspilcious I think the main improvement my code makes (BTW, you're code is much better than my attempt) is that I added a GraphInterface that was super simple and only included definitions of the public functions and not the DepthFirstSearch code.

I then implemented the interface with a DepthFirstSearch.php. That way others could write their own implementations of the interface (ex: BreathFirstSearch, BestFirstSearch, etc.)

Do you think those additions are valuable?

I think thats a giant overkill for this at this moment.

I really hate to be that guy but I thought we had a discussion ongoing and then patches appeared and then it became RTBC even though even #15 is not implemented not to mention #16. How can you "just switch to PSR-0" when something was not a class (and is still not functionally) ? #15 however, did make sense. Some. Not that this whole issue makes a lot of sense. Why? #16 didn't really get an answer. If autoloading is what you are after add a 1-line function to something that always loads and load it from there. End of story. This is not a class. This was not meant to be a class. We do not need a class. I have made amends with this new OOP zealotry but there is a limit and this is it. OOP has its place. When it's sane. You need a reasoning why you convert something to a class. Interface is one. Cross that off, we do not need another implementation for this, the data structure will never go so huge you want to turn to neo4j or somethin'. Encapsulation is another. Cross that off too because there is nothing to encapsulate. What else...? Tell me.

As this is slated as a Component, having a namespaced class is the industry standard way to create sharable, reusable code. It makes it dead simple to mix and match and recombine. The Component part of the namespace is specifically set aside for precisely that, by design. Even if few if any people do split off code from it to use stand-alone, designing our code in such a way is good architectural practice.

Additoinally, if Drupal 8 is going to be a predominantly OO system (as it appears is likely), then for code where "meh, it could go either way", it will be easier for people if it is OO since it will be more familiar than having a stray procedural function lying about, something that would be unexpected for every PHP developer in the world who has worked with something other than Drupal or Wordpress.

Plus, functions are inherently difficult to impossible to unit test at any decent scale. Objects are much easier to inject, and therefore unit test. (Or, rather, the code that uses them becomes easier to unit test.) Improving our structure to make unit testing easier makes for fewer bugs, faster test runs, and a more reliable code base.

We want autoloading, and that requires a class.

if (!function_exists('drupal_depth_first_search')) {
  require_once 'path/to/graph.inc';
  drupal_depth_first_search($graph);
}

It's not autoloading, but it does get around the problem that we're trying to solve with autoloading, which is that we don't want a bunch of crap in memory that we're not going to use.

OK #27 is something I could live with. Not happy but I promised I will get with the program. Final bikeshedding and I know this was in the original as well (someone file a D7 issue plz): "* Performs a depth-first sort on the directed acyclic graph." wtf is that. There's no such animal. We perform a depth-first search and derive a topological sort of the graph from that. Figure out please better doxygen and method names. I am on IRC to help.

If we want a patch that converts graph.inc to PSR-0 then this is it. However this comment does not endorse this issue.

This looks like the natural direction to go in. Committed to 8.x. Thanks all.

I think backslashes got removed in the notification.

Yeah, that's a PHP 5.2 bug. It'll get fixed when Drupal.org moves to PHP 5.3. #1322960: No support for PHP namespaces

I fixed the change notification for now by using code tags instead of PHP tags.

Let's go ahead and backport the documentation fixes that apply to D7.

I believe these are the relevant docs changes for D7.

OK, made a couple more adjustments to the patch after reading through the D8 patch again. I think this should about cover it.

#39: 1503184-depthfirstsearch-39.patch queued for re-testing.

That looks in-line with D8. (Neither of them is perfect but it's the same as D8 at least and way, way better than the nonsense we have right now)

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/31a4cbd

Restoring something resembling the original issue status.

That looks like it still needs to be ported, I think this should stay as 7.x.

What's the part that needs to be ported? The PSR-0 stuff is obviously only D8, and I thought the documentation fixes above were the only things being backported. Or were there parts of it that were missed?

Sorry, I meant that normally issues tagged with "needs backport to D7" are left against 7.x after being backported. Though as only half of this got backported, so I'm not sure.

I occasionally check http://drupal.org/project/issues/search/drupal?status[]=2&version[]=8.x&... to make sure everything got taken care of.

Only the docs needed a backport in this case.