Problem/Motivation
In order to remove all of the hardcoded filter IDs from the Svelte app, #3280176: Decouple front-end (Svelte) from "mockapi" to allow other plugins allows each plugin to provide the values for different filters via additions to ProjectBrowserSourceInterface:
getDevelopmentStatuses()
getMaintenanceStatuses()
getSecurityCoverages()
For example, here's MockDrupalDotOrg's implementation for maintenance statuses:
public function getMaintenanceStatuses(): array {
return [
['id' => 13028, 'name' => 'Actively maintained'],
['id' => 19370, 'name' => 'Minimal'],
['id' => 9990, 'name' => 'Seeking co-maintainer'],
['id' => 9992, 'name' => 'Seeking new maintainer'],
['id' => 13032, 'name' => 'Unsupported'],
];
}
This is good because it keeps the magic numbers out of the rest of the app.
However, it means that any plugin can return any filters it wants.
What we really want is a standardized list of statuses, and for each plugin to provide their own mapping.
Steps to reproduce
Proposed resolution
Remaining tasks
- ✅ File an issue about this project
- ☐ Addition/Change/Update/Fix to this project
- ☐ Testing to ensure no regression
- ☐ Automated unit/functional testing coverage
- ☐ Developer Documentation support on feature change/addition
- ☐ User Guide Documentation support on feature change/addition
- ☐ Code review from 1 Drupal core team member
- ☐ Full testing and approval
- ☐ Credit contributors
- ☐ Review with the product owner
- ☐ Release
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork project_browser-3281218
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
fjgarlin commentedAPPROACH 1
One idea is to have a "Utility" class that checks for certain labels, and then we can call that class from the Controllers, once the data has been retrieved. If the check goes well, then we continue as it is. But if it's not, what shall we do?
* A user message won't be helpful as the user won't be able to do anything to fix it
* Not rendering any data seems a bit drastic but I guess it is what we want here. If in the future we have multiple plugins bringing information, I guess we'll just ignore the data coming from plugins that fail this check.
So, while this issue makes sense, I think it's too early to implement and might not be even needed at all (see the below).
--
APPROACH 2
We might want to have a "primary" plugin and then "secondary" plugins, and then force these to follow what the primary dictates (most likely drupal.org). Somehow "hardcoding" additional checks in this module that could even make the d.org plugin fail if it ever changes a label feels inappropriate as we should trust fully the (future) default plugin and its data.
I think that it would make sense to have the default plugin dictate the values that will be needed from other plugins (as opposed to having this module doing it).
So:
* If only one plugin is enabled, everything works as it works now, no additional checks.
** ie: gov sites might only want to allow gov-approved modules and they can create their own plugin. This module could handle that perfectly.
* If more than one plugin is enabled (future issue).
** Define a "primary" plugin to read the filters from. That'll be the "source of truth".
** All filters are still read from the main plugin, so no change in the front-end and we don't even query the secondary plugins for filter data.
** Each "secondary" plugin should implement a "map" when querying the data to convert vocabularies.
** Add the possibility for plugins to be declared "primary only". I'm thiking that d.org plugin will never want to adapt to any other plugin.
Happy to listen your thoughts on the above.
Comment #3
drummWhile these statuses have been stable on Drupal.org, we need the flexibility to change them at any time, without core coordination.
Many issues have been opened over the years about how they are not the best filters, such as #2235021: Add additional granularity to "development status" on projects, #1129394: Allow multiple values in project "Maintenance status" field, #2235021: Add additional granularity to "development status" on projects, #1134450: Automatically degrade maintenance and development status of projects over time. They are indeed non-ideal and not always useful to site builders. We don’t have any specific changes on the horizon, but they are likely to happen in the long run. Currently we can execute this sort of change quickly, once we’ve settled on what to do. We always should be able to do this, and provide immediate benefit to people searching for projects.
Comment #4
tim.plunkettRe: #3, this may be an incredibly naive question as a monolingual English speaker, but is d.o translated at all?
Because Drupal Core is, and unless there's a mechanism for retrieving translated versions of the filter strings, I'm not sure how we address that disconnect.
Comment #5
chrisfromredfinDo we need to understand the filters from the front-end perspective only, and then let the plugin decide how we deal with them?
For example, maybe the frontend filters are something like:
- Security: covered by a security policy / not covered by a security policy
- Maintenance: Well-Maintained / not maintained
- Development: Active / Stale
Then we just allow people to include stale, not maintained, or not covered. So on the frontend the user just sees these as more like bools, and then the plugin can decide what status means what. The d.o backend plugin could fetch the list and tids, but I honestly don't know how to do that without having to change SOME code. If we kept the terms the same, that would work - because the tids would stay the same... but if we added something new, I don't see a way around having to update the PB backend...
Comment #6
tim.plunkettI'm really interested in #5 as an approach, and also 100% agree with the point about tids vs labels.
Unassigning myself for now
Comment #7
run fast think slow -lets4- commentedWe don't need to check 'has been replaced' or 'project type=full'?
We are checking the release versions in another place as well?(make sure not getting D6)
Comment #8
tim.plunkett#7
https://git.drupalcode.org/project/project_browser/-/blob/1.0.x/project_...
https://git.drupalcode.org/project/project_browser/-/blob/1.0.x/project_...
Comment #9
fjgarlin commentedSince #5 would mean changes to front-end, controllers and plugins, I'll take this one as it's a similar exercise to when I "decoupled" things in https://www.drupal.org/project/project_browser/issues/3280176
Comment #10
fjgarlin commentedPossible suggestions for filters. From slack conversation here: https://drupal.slack.com/archives/C01UHB4QG12/p1654092626909329
Radio buttons with the following:
Comment #11
drumm“Well-Maintained” is not a good label, since it is very subjective. Many of the indicators we have, the maintenance and development status, have options that are good negative indicators. “Actively maintained” and “Under active development” are at best neutral, and shouldn’t be highlighted as positive indicators.
Comment #12
fjgarlin commentedBased on the slack conversation mentioned in #10 and comment in #11, we'll remove the word "Well-".
Comment #13
fjgarlin commentedhttps://www.drupal.org/project/project_browser/issues/3284347#comment-14...
I still haven't started on this as there are a few issues working on the same components that this will modify. Also, comments like the one linked above still makes me thing that the "main" plugin could dictate what the values should be for now.
In any case, I'm keeping a close eye on the issue queue to see when it can be a good moment to tackle this.
Comment #15
fjgarlin commentedI think this covers the backend side of things. I'll get going with the frontend tomorrow.
Comment #16
fjgarlin commentedThis is a big one and it’s ready to review. Assigning to @tim.plunkett for now as we already discussed a while back about this issue.
I’m ok doing a zoom call if needed as it touches files all over the place.
ProjectBrowserSourceBase has “the contract”. It’s an abstract class with some abstract methods and some final methods. That’s the starting point for all the other changes
Comment #19
tim.plunkettThanks for addressing all my feedback. Merged!