#2050289: Add class to make yaml discovery reusable changed how routing.yml files are discovered. Instead of the module name as before, it now uses the basename of the directory.

This broke having a ui folder with a $mymodule_ui.module and a lot of similar use cases.

Files: 
CommentFileSizeAuthor
#19 2067809-19.patch8.21 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,840 pass(es).
[ View ]
#19 interdiff-2067809-19.txt1.33 KBdamiankloip
#13 2067809-13.patch6.88 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,427 pass(es).
[ View ]
#13 interdiff-2067809-13.txt1.33 KBdamiankloip
#12 discovery-module-name-2067809-12-test-only.patch4.14 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,642 pass(es), 31 fail(s), and 1 exception(s).
[ View ]
#12 discovery-module-name-2067809-12.patch5.9 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,420 pass(es).
[ View ]
#1 discovery-module-name-2067809-1.patch6.08 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,832 pass(es).
[ View ]
#1 discovery-module-name-2067809-1-test-only.patch5.9 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,327 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new5.9 KB
PASSED: [[SimpleTest]]: [MySQL] 58,327 pass(es).
[ View ]
new6.08 KB
PASSED: [[SimpleTest]]: [MySQL] 58,832 pass(es).
[ View ]

For this to fix we need to make the fact that the array keys are the module names (or actually provider, to stick with the term used in annotation plugin discovery) explicit.

Renamed the router_test folder to make the tests fail without the fix.

Status:Needs review» Needs work

The last submitted patch, discovery-module-name-2067809-1-test-only.patch, failed testing.

Issue tags:-Needs tests

This killed the phpunit tests because it makes the same wrong assumption, there already was #2025883: Drupal's PHPUnit bootstrap.php does not register module namespaces out of /core but that didn't fix this yet either, updated the patch there accordingly.

I'm not crazy about this. It adds complexity to YamlDiscovery for the sake of supporting modules contained in directories that are arbitrarily named? I'm not sure why we even support that.

We did until this landed, discussed this with @alexpott, he agrees this is a bug. I don't see the complexity (it is less code than before), instead of the undocumented folder name behavior, it now uses the documented provider/array key?

More code != complexity. It now requires a more complex data structure which introduces the concept of a "provider". Not a huge difference, sure, but it's there.

The idea of the fileBaseName method was that if the basename($directory) logic didnt work, you could easily override it with that one method. I'd almost prefer to see a Drupal\Core subclass of this that got the basename from the info.yml file.

I don't dislike this enough to argue about it much more, it just seems like a completely unnecessary feature to maintain.

Component:dashboard.module» plugin system

Well, it's not really a unecessary feature when it worked fine before this refactoring :)

This is a very confusing behavior right now, because you can enable those modules and they work just fine... except routing. Had to do quite a bit of debugging to find out what's going on.

I would be ok if we'd generally enforce directory name = module name but that's not for this class to decide :) And it is probably too late for such a change, when not strictly necessary.

Not actually the plugin system but don't see anything better and it'll affect it eventually #2065571: Add YAML Plugin discovery. Another thing to note, "module" isn't entirely correct because it could be a theme if the system injected themes. Something plugins will probably do in some cases.

Use case where this could be confusing, cloning sandboxes clones to a directory named as the node id. This would lead to broken modules unless the person cloning the repo knows to change the directory to be the same name as the module. Kinda yucky.

Just as a note: If this is *not* going to be fixed, then we should definitely put a note in the README file in the top-level modules directory, because the practice of putting multiple modules in the same directory is obviously very widespread in D7. (like: all over contrib for instance).

Status:Needs work» Needs review

#1: discovery-module-name-2067809-1.patch queued for re-testing.

StatusFileSize
new5.9 KB
PASSED: [[SimpleTest]]: [MySQL] 58,420 pass(es).
[ View ]
new4.14 KB
FAILED: [[SimpleTest]]: [MySQL] 58,642 pass(es), 31 fail(s), and 1 exception(s).
[ View ]

The test-only patch was actually not test-only at all. This should fail, also re-uploading the full patch for clarity.

StatusFileSize
new1.33 KB
new6.88 KB
PASSED: [[SimpleTest]]: [MySQL] 58,427 pass(es).
[ View ]

Me and berdir spoke about this on IRC, just making some small (pretty much docs) tweaks to this class now, so it's all taken care of.

The rest of the patch looks good to go.

Did you say docs? [jhodgdon pokes head up] :)

The docs look good in #13, assuming that the term "provider" means something... As someone coming to this one class by itself, it might not be obvious what "provider" means (it wasn't to me, for instance). I'm sure it's standard Drupal 8 terminology somewhere, but it doesn't make the docs for this particular class very illuminating, I have to say.

We are generally using 'provider' now in a lot of places (like the plugin system), as the term module is not always accurate anymore.

I'm not sure what you want me to say really.. :) We can't really add the story of how we arrived at provider. You could say something similar about any term/word used for almost anything I guess.. Also, remember this is generic class in the Component directory.

Maybe we need an @ingroup for "List of Drupal's specific terminology". As a community, we coin these terms like "node" and "provider" and "entity" that only have meaning if you happen to be in the "in crowd" of Drupal developers. But just because everyone has agreed to use these terms, that doesn't mean that people new to Drupal will understand what they mean.

I guess I'll file a separate issue to discuss this problem though; I didn't change the review status of this patch, and the problem is much more widespread (obviously) than this one patch.
#2104837: There are lots of drupalisms used in API docs and never explained

Ok, sounds good. Agree, this is not the problem of this patch as such.

Something like provider is not that much of a mysterious term though in my opinion.

I think it would be great to have an explicit test somewhere which tests that the route_test module is actually loaded. Yes things will fail completly without it, though having tests is kind of automatic documentation.

StatusFileSize
new1.33 KB
new8.21 KB
PASSED: [[SimpleTest]]: [MySQL] 58,840 pass(es).
[ View ]

Well, we can just add this to the YamlDiscovery unit test :)

Status:Needs review» Reviewed & tested by the community

Nice!!

Since we've got a unit test do we need to move the other test?

On #2104837-4: There are lots of drupalisms used in API docs and never explained, it was pointed out that there are a bunch of different types of "providers" in Drupal Core. So should we be being more specific about what "provider" means here?

In this case, all it means is the first part of the file name, as the $name param above documents. The class itself doesn't care at all what it is, it just uses the provided string to build the file name it's supposed to look for.

IMHO, this is ok, but if someone has suggestions on how to document that better, feel free to provide an updated patch :)

I agree this is good as is. This doesn't seem like the place to start trying to give our story of what a provider is. Plus the current docs explain it's use.

The whole provider thing almost seems like it could live in a change notice or something (d.o in general)?

Status:Reviewed & tested by the community» Fixed

We've introduced 'extension' elsewhere to deal with things that might or might not be modules/themes/install profiles, at some point we should reconcile that with provider if they're actually the same thing - not sure they are though in the context of the component since it doesn't care.

Patch looks fine as it is though. Committed/pushed to 8.x, thanks!

Status:Fixed» Closed (fixed)

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