In the function definition, I commented
@todo this probably does not need to be done client side. Moving this to
logic that updates drupalSettings in BrowserController can potentially do
this with less complexity. The one hurdle is adding the conditions for
adding this in a manner that does not fail if Package Manager isn't
installed.
Somebody (me) built this as if the compatibility check and the Svelte app were on different sites, but it's the same site and we don't need to deal with fetch()-ing
Show commandsStart 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:
changes
Comments
Comment #2
narendrarComment #4
narendrarComment #5
bnjmnmComment #6
bnjmnmComments in MR
Comment #7
narendrarComment #8
bnjmnmI reduced the overall number of files that needed to be changed, and tests don't have to be changed at all. Because of that this will require a review from someone other than me.
Comment #9
fjgarlin commentedI love that we’re moving that logic to the controller, that’s totally where it should be.
I left a couple of comments, one really minor and one perhaps a bit more complex. I think that rather than using the controller as a service, we can extract what’s needed into an "install_readiness" service, and then inject that service into the controllers.
Comment #10
narendrarComment #11
fjgarlin commentedThe code looks good to me. I love that we're moving the logic to services and controllers and just pass it to the front-end.
I tried to test it on drupalpod but it didn't like the symlinks, so I tested locally and it all works as expected.
I'll probably create a new ticket for the symlinks, as I feel that we shouldn't be able to click on "Allow UI capabilities" if we know that it's not going to work. Package manager does this check so we might be able to use it.
As for this issue, I'm marking it RTBC.
Comment #12
fjgarlin commentedI'm not sure which part of the code made the regression but see this issue #3328879: Do not offer "Allow installing via UI" if readiness checks fail. I created it thinking it that it was on the main dev branch, but it only happens here on this issue fork.
Basically, if there is a package_manager issue it should show up in the main PB browsing page, like here:
But it does not happen, the message does not show up in the PB browsing page, even if it shows in the status report. This leads to users not seeing the warning, clicking on "add and install" and leading to this:
So I think that we need to bring back the warning. Marking it again as NW.
Comment #13
narendrarComment #14
fjgarlin commentedReview the code ✅
Tested on drupalpod, saw the error about the symlink ✅
Tested locally, saw error about symlink, then removed symlink and saw no error, and could install modules via UI ✅
Marking again RTBC. Thanks so much for this refactoring!
Comment #17
tim.plunkettMerged, thanks!