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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Status: Active » Needs review
FileSize
5.9 KB
6.08 KB

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.

Berdir’s picture

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.

msonnabaum’s picture

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.

Berdir’s picture

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?

msonnabaum’s picture

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.

Berdir’s picture

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.

neclimdul’s picture

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.

jhodgdon’s picture

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

Berdir’s picture

Status: Needs work » Needs review
Berdir’s picture

Berdir’s picture

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

damiankloip’s picture

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.

jhodgdon’s picture

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.

damiankloip’s picture

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.

jhodgdon’s picture

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: Add a glossary of Drupal terms to the API documentation

damiankloip’s picture

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.

dawehner’s picture

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.

damiankloip’s picture

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Nice!!

neclimdul’s picture

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

jhodgdon’s picture

On #2104837-4: Add a glossary of Drupal terms to the API documentation, 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?

Berdir’s picture

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 :)

damiankloip’s picture

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

catch’s picture

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.