Files: 
CommentFileSizeAuthor
#39 1503184-depthfirstsearch-39.patch1.01 KBDave.Ingram
PASSED: [[SimpleTest]]: [MySQL] 39,062 pass(es).
[ View ]
#38 1503184-depthfirstsearch-38.patch558 bytesDave.Ingram
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to drop checkout database.
[ View ]
#30 1503184.patch29.65 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 36,149 pass(es).
[ View ]
#27 1503184.patch29.91 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 36,165 pass(es).
[ View ]
#21 1503184-depthfirstsearch-21.patch31.17 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 35,890 pass(es).
[ View ]
#19 1503184-depthfirstsearch-19.patch31.16 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 35,887 pass(es).
[ View ]
#12 1503184-graph-12.patch22.07 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 35,910 pass(es).
[ View ]
#10 1503184-graph-9.patch18.83 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 35,918 pass(es).
[ View ]
#7 1503184-graph-7.patch18.72 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 35,766 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#4 1503184.patch17.27 KBcweagans
FAILED: [[SimpleTest]]: [MySQL] 35,744 pass(es), 47 fail(s), and 21 exception(s).
[ View ]
#3 1503184.patch17.3 KBcweagans
FAILED: [[SimpleTest]]: [MySQL] 35,727 pass(es), 47 fail(s), and 21 exception(s).
[ View ]
graph.patch16.68 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

Status:Needs review» Needs work

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

Crell’s picture

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.

cweagans’s picture

StatusFileSize
new17.3 KB
FAILED: [[SimpleTest]]: [MySQL] 35,727 pass(es), 47 fail(s), and 21 exception(s).
[ View ]

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.

cweagans’s picture

Status:Needs work» Needs review
StatusFileSize
new17.27 KB
FAILED: [[SimpleTest]]: [MySQL] 35,744 pass(es), 47 fail(s), and 21 exception(s).
[ View ]

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

Status:Needs review» Needs work

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

Crell’s picture

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.

aspilicious’s picture

Status:Needs work» Needs review
StatusFileSize
new18.72 KB
FAILED: [[SimpleTest]]: [MySQL] 35,766 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

aspilicious’s picture

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

Status:Needs review» Needs work

The last submitted patch, 1503184-graph-7.patch, failed testing.

aspilicious’s picture

Status:Needs work» Needs review
StatusFileSize
new18.83 KB
PASSED: [[SimpleTest]]: [MySQL] 35,918 pass(es).
[ View ]

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

aspilicious’s picture

And we shouldn't camelcase variables....

aspilicious’s picture

StatusFileSize
new22.07 KB
PASSED: [[SimpleTest]]: [MySQL] 35,910 pass(es).
[ View ]

Updated patch...

RobLoach’s picture

Status:Needs review» Reviewed & tested by the community

I'm okay with Graph being an instantiated object rather than a static class function. Hitting the RTBC button.

chx’s picture

Status:Reviewed & tested by the community» Needs work

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.

RobLoach’s picture

Is this what you're asking for?

<?php
use Drupal\Component\Graph\Graph;

$graph = new Graph($graph_array);
$result = $graph->search();
?>

The name? Would "DepthFirstSearch" instead of "Graph" be good?

chx’s picture

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

cosmicdreams’s picture

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.

Crell’s picture

#15 makes sense to me.

aspilicious’s picture

Status:Needs work» Needs review
StatusFileSize
new31.16 KB
PASSED: [[SimpleTest]]: [MySQL] 35,887 pass(es).
[ View ]

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

Tor Arne Thune’s picture

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

aspilicious’s picture

StatusFileSize
new31.17 KB
PASSED: [[SimpleTest]]: [MySQL] 35,890 pass(es).
[ View ]

ARGH!

I'll never learn it...

cosmicdreams’s picture

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

aspilicious’s picture

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

RobLoach’s picture

Status:Needs review» Reviewed & tested by the community

Yeah, this issue is just to switch it to PSR-0. We could expand upon that in a follow up issue though! This looks good to me.

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

You might also enjoy #1497230: Use Dependency Injection to handle object definitions and #1497366: Introduce Plugin System to Core ;-) . With either Dependency Injection or Plugins, we could make it swappable. You can even swap the class using the ClassLoader, but that's the ugly solution.

chx’s picture

Status:Reviewed & tested by the community» Needs work

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.

Crell’s picture

Status:Needs work» Reviewed & tested by the community

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.

RobLoach’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new29.91 KB
PASSED: [[SimpleTest]]: [MySQL] 36,165 pass(es).
[ View ]

#25: even though even #15 is not implemented

The attached patch implements #15.

Leaving alone the include file would be even better. I fail to see the object here.

Autoloading.

It's not pluggable either.

Like I mentioned earlier, with #1497230: Use Dependency Injection to handle object definitions , or #1497366: Introduce Plugin System to Core, we could move in that direction. It's also possible to swap it out with the ClassLoader. Using #1497230: Use Dependency Injection to handle object definitions or #1497366: Introduce Plugin System to Core are the "better" solutions.

#16: Why are we converting this file...? Autoloading? Sure...

Yes, we want to move this over to PSR-0 autoloading.

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.

We want autoloading, and that requires a class.

#25: #15 however, did make sense.

Hopefully you enjoy this patch then! It implements the suggestion at #15, which you endorsed.

[EDIT] Ah, cross-posted with Crell. Well, let's see what the others and the test-bot think of this one.

cweagans’s picture

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.

chx’s picture

Status:Needs review» Needs work

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.

RobLoach’s picture

Status:Needs work» Needs review
StatusFileSize
new29.65 KB
PASSED: [[SimpleTest]]: [MySQL] 36,149 pass(es).
[ View ]

Talked with chx on IRC a bit. We came up with a few conclusions:

  1. chx: similar to what the oirignal file doxygen suggested -- there could've been more. so this +use Drupal\Component\Graph\Graph; is better.
  2. "Directed acyclic graph functions." doesn't make sense for the class, Use "Directed acyclic graph manipulation." for the class docs.
  3. search() isn't all that the function does. It also sorts... so change to searchAndSort()
  4. Change "Perform the actual sort." to "Perform the actual search."
  5. Change sort() function to depthFirstSearch()
chx’s picture

Status:Needs review» Reviewed & tested by the community

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

Dries’s picture

Status:Reviewed & tested by the community» Fixed

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

aspilicious’s picture

Title:Convert Graph.inc to PSR-0» Change notification for: Convert Graph.inc to PSR-0
Priority:Normal» Critical
Status:Fixed» Active
RobLoach’s picture

Status:Active» Needs review

http://drupal.org/node/1525776

chx also made a good point that the Doxygen fixes could be backported to Drupal 7.

aspilicious’s picture

I think backslashes got removed in the notification.

webchick’s picture

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

xjm’s picture

Title:Change notification for: Convert Graph.inc to PSR-0» Convert Graph.inc to PSR-0
Version:8.x-dev» 7.x-dev
Priority:Critical» Normal
Status:Needs review» Patch (to be ported)
Issue tags:+needs backport to D7

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.

Dave.Ingram’s picture

Title:Convert Graph.inc to PSR-0» Make Graph.inc documentation clearer.
Assigned:Unassigned» Dave.Ingram
Status:Patch (to be ported)» Needs review
StatusFileSize
new558 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to drop checkout database.
[ View ]

I believe these are the relevant docs changes for D7.

Dave.Ingram’s picture

StatusFileSize
new1.01 KB
PASSED: [[SimpleTest]]: [MySQL] 39,062 pass(es).
[ View ]

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

Status:Needs review» Needs work
Issue tags:-needs backport to D7, -PSR-0

The last submitted patch, 1503184-depthfirstsearch-39.patch, failed testing.

aspilicious’s picture

Status:Needs work» Needs review
Issue tags:+needs backport to D7, +PSR-0

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

chx’s picture

Status:Needs review» Reviewed & tested by the community

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)

David_Rothstein’s picture

Title:Make Graph.inc documentation clearer.» Convert Graph.inc to PSR-0 (and make its documentation clearer)
Version:7.x-dev» 8.x-dev
Status:Reviewed & tested by the community» Fixed

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

Restoring something resembling the original issue status.

tim.plunkett’s picture

Version:8.x-dev» 7.x-dev

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

David_Rothstein’s picture

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?

tim.plunkett’s picture

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

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.

aspilicious’s picture

Only the docs needed a backport in this case.

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