- Issue created by @hestenet
- First commit to issue fork.
- 🇺🇸United States chrisfromredfin Portland, Maine
This is an interesting concept. It also advances us toward potentially replacing the existing "List" tab of the module page now.
Removing the "array_filter()" at the top of getCoreModules() gets us really close as a proof of concept. But things to be decided... for modules that ARE part of drupal.org, how could we detect that and fetch the correct relevant data for them (security coverage, number of installs, etc). We could still use the categories that are present on the existing List page, but they would then be _different_ from if you were browsing on the drupalorg_jsonapi (something brought up frequently in early UI discussions - i.e. can we make all those categories MATCH?)
Also, doing this scan might bring up custom modules that don't have anything to do with drupal.org.
One possible answer is to stash some of this kind of metadata in the .info.yml. For drupal.org projects, maybe that happens with packaging script - like writing out the category or something, but that may open up a can of worms also?
If anyone has any thoughts on these, I'd be interested in hearing them. :)
- 🇮🇳India narendraR Jaipur, India
Should a new plugin listing locally available Core, Contrib, and Custom modules be implemented?
- This plugin will exclude test modules and sub-modules.
- It will support all filters from the drupalorg_jsonapi plugin.
- Sorting options: A-Z, Z-A.
- Category filtering will include options from both drupal_core and drupalorg_jsonapi plugins.
- 🇪🇸Spain fjgarlin
Filters and category filtering might prove tricky unless you bring up all the information locally and then do all the filtering based on that.
I'm not sure that filtering would be needed if the module is already in the codebase. If we need/want some, I'd just do what the "core" plugin is doing, which is checking the "package" property for the categories.
- 🇺🇸United States chrisfromredfin Portland, Maine
The more I think about it, the more I think that we need to provide a "local contrib" plugin, and not commingle custom, contrib, and core into one backend. They're just a bit too different to do that. If we _have_ to, then we could merge custom and contrib, but I still don't want to. Here's why:
- We already have a core plugin that works.
- A contrib ProjectBrowserSourcePlugin could be made that scans a given folder (perhaps this is configurable? defaulting to web/modules/contrib), and then fetches and caches metadata for those projects from Drupal.org. In this way we can provide the best fully-featured experience for the trial experience but it restricts the ability to download by the nature of it only showing ones locally available.
- A "custom" ProjectBrowserSourcePlugin could be made that operates much the same way, but uses data from the .info.yml, much like the core plugin does today.
I see this being a good UI, where each tab is Core | Contrib | Custom. (I would really want to get in ✨ Expose sources as local actions underneath a "Browse" local task Active )
ALSO, I want to point out that there's a facility to write these plugins in a separate contrib module right now, nothing is stopping anyone. I wonder if that's the best way to pioneer this for the Trial Experience benefit, tho the way I've architected it above, I would certainly consider it as an MR.
And, by no means is this the end of the discussion, I'm sure I'm thinking of a number of things I'm still not thinking of. :)
- 🇪🇸Spain fjgarlin
there's a facility to write these plugins in a separate contrib module right now
100% - creating a new "source" or "plugin" via a contrib module is extremely easy.
- 🇮🇳India narendraR Jaipur, India
Basic functionality is ready. This MR needs tests and a way to filter development status.
- First commit to issue fork.
- 🇺🇸United States phenaproxima Massachusetts
Crediting @fjgarlin for critical insight that will make this a lot easier.
- 🇪🇸Spain fjgarlin
I see great progress on this. Right now there is still a lot of code duplication between
src/Plugin/ProjectBrowserSource/DrupalDotOrgJsonApi.php
andsrc/Plugin/DrupalDotOrgSourceBase.php
.I assume that we are moving a big chunk to
DrupalDotOrgSourceBase
and that everything is still WIP, so I'll hold of until I see it's marked as "Needs review". - 🇺🇸United States phenaproxima Massachusetts
@fjgarlin, the idea is to move anything that can be generalized as part of "how to talk to d.o" into
DrupalDotOrgSourceBase
, and the more specific choices about what to ask d.o into the plugin. I didn't see a ton of duplication when I worked on it, but maybe I'm missing something; can you give a few examples of the duplication you're seeing?Where testing is concerned, I think that, since the local modules plugin is a simple decorator, all we need here is a kernel or unit test. We need to confirm that the $query passed to the decorated plugin always has a machine_name field that is a comma-separated list of the currently installed modules. We can accomplish this with a couple of simple mock objects.
- 🇪🇸Spain fjgarlin
Also, if
LocalModules
is not extendingDrupalDotOrgSourceBase
, why do we even need to splitDrupalDotOrgJsonApi
into two classes? - 🇺🇸United States phenaproxima Massachusetts
Replying to #17 -- this was recommended by @tim.plunkett as a good future proofing step for when (it's probably not a question of "if") we want to add more plugins that query drupal.org in opinionated ways. We were able to do a decorator this time, but that might not always be the case. I personally don't see too much harm in splitting the d.o API parts into a base class but we could revert that if you feel really strongly about it.
- 🇪🇸Spain fjgarlin
No strong feelings, I was mostly questioning because it was (a) just one decorator and (b) the code duplication. If the plan is to have more decorators down the line, that's all good with me.
My only concern is the duplication of code. Right now, these functions have the exact same code in both places:
- fetchData
- mapIncludedData
- getFilterDefinitionsAnd then this one is split into two functions in the "DrupalDotOrgJsonApi", but the code is mostly the same
- getCategories - 🇺🇸United States tim.plunkett Philadelphia
#21 all three of those methods are being removed from DrupalDotOrgJsonApi and as of this MR now only exist on DrupalDotOrgSourceBase.
- 🇪🇸Spain fjgarlin
Thanks for the cleanup! Code-wise, it's got the thumbs up from my side. I haven't manually tested it tho.
- 🇪🇸Spain fjgarlin
So, if I do
composer require drupal/webform drupal/address
, the only way to see these modules if after clearing the storage (/admin/config/development/project_browser/actions
). I understand why this is like that, but it seems like this new plugin shouldn't be cached. It can be a follow-up I guess, or just leave it as it is, but I'm sure that it'll be confusing to people if they add modules locally and they don't show up directly. - 🇺🇸United States phenaproxima Massachusetts
Opened ✨ EnabledSourceHandler's query result caching should also consider the contents of composer.lock Active to address #26 separately. I'm guessing it should also be an alpha10 should-have.
- 🇪🇸Spain fjgarlin
Thanks for opening the follow-up. As for this issue then, RTBC++.
- 🇺🇸United States chrisfromredfin Portland, Maine
Manual testing revealed a couple things for me:
(1) it had trouble saving the config screen for me initially; however, not sure if this is reproducible. A re-installed cleared this up, so maybe was just a weird state I had it in... but I do think I had it pretty cleanly installed. Race condition???
(2) Every module I try to install that's available locally shows me: "RuntimeException: Project 'drupalorg_jsonapi/drupal-easy_breadcrumb-easy_breadcrumb' was not found in non-volatile storage."
NOTE: If I am sure to pull up that project somehow by using the "Contrib modules" source first, then it works. But it seems like this module is either not namespacing its OWN non-volatile storage, or is simply trying to request/install from the wrong source. This source should be able to stand on its own without dependency on the Contrib modules one.
- 🇺🇸United States phenaproxima Massachusetts
Ooh, good find. Yeah - it wasn't properly populating the non-volatile project store because the results page was still returned under the name of the decorated plugin.
That's fixed now and there's test coverage to prove it. I also gave it some local manual testing and it worked as intended.
- 🇺🇸United States chrisfromredfin Portland, Maine
Re-tested, working reliably. Tested in Gin and Claro, max_selections NULL & 1. With and without 'Contrib modules' plugin enabled, etc.
-
chrisfromredfin →
committed 2e63ded7 on 2.0.x authored by
narendrar →
Issue #3498835: Feature: add plugin or option to show only locally...
-
chrisfromredfin →
committed 2e63ded7 on 2.0.x authored by
narendrar →
- 🇺🇸United States chrisfromredfin Portland, Maine
Amazing work! A testament to the flexible system we have going on here.
Automatically closed - issue fixed for 2 weeks with no activity.