backdrop / backdrop-issues

Issue tracker for Backdrop core.
145 stars 40 forks source link

[DX] Project installer: Do not try to fetch release information about submodules that have been added as dependencies - only download their parent modules instead #6607

Open stpaultim opened 5 months ago

stpaultim commented 5 months ago

Description of the need

I've created a Recipe Package that basically wraps together a number of other recipes and some config and sample content to deliver the framework for a working website. In this case, it's called Digital Agency.

Several of the recipes have "sample content" sub-modules. The sample content is in sub-modules to give folks the change to decide if they want the sample content or not. This seems to make sense when installing the recipes on their own.

However, as part of the Digital Agency recipe package, the sample content should probably be required and in fact, if we don't list these sample content modules as dependencies, they end up loading out of order and causing problem.

(EDIT) When I list the submodules as dependencies, everything works exactly as I would expect. Except, because they are sub-modules, I get the following warning during installation if the submodules are listed as dependencies.

It seems as if Backdrop is confused about where to find these sub-modules, because it can't fetch them on it's own, when in fact they have been fetched as part of their parent module. Again, everything works fine, except for the unnecessary warning, which may confuse folks.

Could not fetch releases for project services_sample_content.
Could not fetch releases for project faq_sample_content.
Could not fetch releases for project portfolio_projects_sample_content.

image

Steps to Reproduce

1) Spin up a demo site. 2) Download and install the Ditigal Agency recipe through the project browser.

Proposed solution

I'm not sure what the best solution is or if a solution is absolutely necessary. But, it would be nice if the installer were able to look inside of other module for these dependencies OR if I could add something in the .info file to specify that these dependencies are sub-modules and tell the installer where to look for them.

Alternatives that have been considered

Just hoping that people ignore the warning.

Additional information

It was suggested that I tried listing the depencies this way (and I have). It did not solve the problem with the warnings.

dependencies[] = portfolio_projects_recipe:portfolio_projects_sample_content

For more info about project:

https://github.com/backdrop-contrib/digital_agency

The benefits of bundling recipes in this way makes it pretty easy to install a single Recipe Package (module) and get something like this:

image

klonos commented 5 months ago

@stpaultim is the problem that you need to add dependencies on submodules included in a parent project, instead of adding a dependency on the parent project itself? Am I understanding this correctly?

klonos commented 5 months ago

...as it is currently, we can declare dependencies in .info files only for the main project itself - we cannot declare a dependency on a submodule, as the Project Installer doesn't have any way to know which project this submodule belongs to (unless the project is already downloaded and exists in the codebase).

So currently we can do the following: dependencies[] = modulename ...what we need is support for something like this instead: dependencies[] = projectname:submodulename ...which would instruct the Project Installer to download and extract projectname, but only enable the submodulename module within the parent/container project (and its sub-dependencies if there are any).

This is not supported currently though - we'd need to implement it. I support this 👍🏼

yorkshire-pudding commented 5 months ago

dependencies[] = modulename ...what we need is support for something like this instead: dependencies[] = projectname:submodulename ...which would instruct the Project Installer to download and extract projectname, but only enable the submodulename module within the parent/container project (and its sub-dependencies if there are any).

According to https://docs.backdropcms.org/api/backdrop/core%21includes%21common.inc/function/backdrop_parse_dependency/1 it can do that, unless I'm misunderstanding.

yorkshire-pudding commented 5 months ago

However, many submodules depend on their parent module so it would normally then make the parent module be enabled too.

avpaderno commented 5 months ago

According to https://docs.backdropcms.org/api/backdrop/core%21includes%21common.inc/function/backdrop_parse_dependency/1 it can do that, unless I'm misunderstanding.

Yes, projectname:submodulename is handled by backdrop_parse_dependency(); it is still to see if the project value returned from that function is used.

klonos commented 5 months ago

Yes, projectname:submodulename is handled by backdrop_parse_dependency();

That's awesome then! 🙂

@stpaultim can you please try changing any recipe that declares dependencies[] = services_sample_content to dependencies[] = services_recipe:services_sample_content instead?

...similarly with:

I believe that that should work.

stpaultim commented 5 months ago

Thanks @klonos, @yorkshire-pudding, and @kiamlaluno for your responses.

I might not have made it clear, that listing the submodules as dependencies DOES WORK in the sense that it does recognize the modules and enable them properly as dependencies. The problem is that during the download process, I assume Backdrop does not know where to find them and provides a warning that it Could not fetch releases for project services_sample_content.

In fact, it's parent module is also a dependency and has been fetched, so in fact the sample content modules has been fetched as well. Backdrop is just confused about this, because it was not able to fetch these dependencies on their own. (At least, this is my assumption about what is happening.)

Other than these warnings, everything works exactly as I would expect. So the only problem is that it's throwing up these warnings which might confuse people.

What would be good, would be if Backdrop actually checked to see if the sub-modules are available within their parend modules, before telling people that it's unable to fetch them, when in fact it does know where they are and is able to fetch them.

I did try listing the sub-module dependencies as @klonos and @yorkshire-pudding suggested and it did not help with these warnings.

klonos commented 5 months ago

@stpaultim thanks for clarifying. Can you please provide some steps to reproduce this? Which set of modules/recipes are you adding via the project browser that then leads to the warnings that you provided in your screenshot?

klonos commented 5 months ago

...never mind. I re-read the issue summary. It's the Digital Agency recipe that is causing those warnings to show up. I have something to work on now 👍🏼

klonos commented 5 months ago

@stpaultim can you please try the following changes in the .info file of the Digital Agency recipe?:

Remove the dependencies on the "parent" modules/recipes:

dependencies[] = mini_layouts
dependencies[] = nice_menus
dependencies[] = contact_us_webform_recipe
dependencies[] = services_recipe:services_sample_content
dependencies[] = faq_recipe:faq_sample_content
dependencies[] = portfolio_projects_recipe:portfolio_projects_sample_content
dependencies[] = testimonial_recipe

...you already have for example the parent faq_recipe module required by the submodule: https://github.com/backdrop-contrib/faq_recipe/blob/1.x-1.x/faq_sample_content/faq_sample_content.info#L7

dependencies[] = faq_recipe

...so it should be enabled as part of the process (as part of requiring its submodule in digital_agency.info). Can you please give that a go?

stpaultim commented 5 months ago

@klonos - I am quite sure that I tried the configuration you suggest in the previous comment. It was the first thing I tried earlier today. This was worse, because using this configuration it required the submodule, but did not download it or the parent module.

I could try this one once more, but not tonight. NOTE: Testing this is problematic, I think, because it's really only an issue when downloading and enabling through the project installer. This means, I have to keep releasing new versions of the recipe to try different things.

I'm not sure if using the manual download works the same way, but I could give that a try too. Thanks for your help.

Yes, (as you figured out) steps to reproduce are:

1) Spin up a demo site. 2) Download and install the Ditigal Agency recipe through the project browser.

klonos commented 5 months ago

This was worse, because using this configuration it required the submodule, but did not download it or the parent module.

OK, then this is not working as intended then ...at least not when it comes to the Project Installer and how it downloads project dependencies. It might be that the project:submodule format only works when enabling already downloaded projects (manually - not via the Project Installer).

I could try this one once more, but not tonight.

Of course. Whenever you get a chance 👍🏼

yorkshire-pudding commented 5 months ago

I don't have an answer to the UI download method, but I have been working recently on this area within bee and will be attempting to ensure have now ensured that this works as expected for the bee download command. This re-write is still in development but this is now the output I'm getting:

 ℹ️ The 'mini_layouts' module will also be downloaded, as it is required by the 'digital_agency' module.
 ℹ️ The 'nice_menus' module will also be downloaded, as it is required by the 'digital_agency' module.
 ℹ️ The 'contact_us_webform_recipe' module will also be downloaded, as it is required by the 'digital_agency' module.
 ℹ️ The 'services_recipe' module will also be downloaded, as it is required by the 'digital_agency' module.
 ℹ️ The 'services_recipe:services_sample_content' submodule is also required by the 'digital_agency' module.
 ℹ️ The 'faq_recipe' module will also be downloaded, as it is required by the 'digital_agency' module.
 ℹ️ The 'faq_recipe:faq_sample_content' submodule is also required by the 'digital_agency' module.
 ℹ️ The 'portfolio_projects_recipe' module will also be downloaded, as it is required by the 'digital_agency' module.
 ℹ️ The 'portfolio_projects_recipe:portfolio_projects_sample_content' submodule is also required by the 'digital_agency' module.
 ℹ️ The 'testimonial_recipe' module will also be downloaded, as it is required by the 'digital_agency' module.
 ℹ️ The 'webform' module will also be downloaded, as it is required by the 'contact_us_webform_recipe' module.
 ℹ️ The 'views_bootstrap' module will also be downloaded, as it is required by the 'faq_recipe' module.
 ✔  'digital_agency' was downloaded into '/app/backdrop/modules/digital_agency'.
 ✔  'mini_layouts' was downloaded into '/app/backdrop/modules/mini_layouts'.
 ✔  'nice_menus' was downloaded into '/app/backdrop/modules/nice_menus'.
 ✔  'contact_us_webform_recipe' was downloaded into '/app/backdrop/modules/contact_us_webform_recipe'.
 ✔  'services_recipe' was downloaded into '/app/backdrop/modules/services_recipe'.
 ✔  'faq_recipe' was downloaded into '/app/backdrop/modules/faq_recipe'.
 ✔  'portfolio_projects_recipe' was downloaded into '/app/backdrop/modules/portfolio_projects_recipe'.
 ✔  'testimonial_recipe' was downloaded into '/app/backdrop/modules/testimonial_recipe'.
 ✔  'webform' was downloaded into '/app/backdrop/modules/webform'.
 ✔  'views_bootstrap' was downloaded into '/app/backdrop/modules/views_bootstrap'.
klonos commented 5 months ago

Thanks @yorkshire-pudding 🙏🏼 ...since we have a way to reproduce the problem, I'm confident that we'll get to the bottom of it. Personally, I'm planning to test this theory I proposed:

It might be that the project:submodule format only works when enabling already downloaded projects (manually - not via the Project Installer).

I'll get to it soon as I get a chance.

avpaderno commented 5 months ago

Truly, the reason for using project:submodule is to make possible to understand which project should be downloaded and installed. Once the module is already copied in the modules directory, there is not need to refer to it as project:submodule because a site cannot have two submodule modules.
It is the code downloading the modules which should use the namespaced name.

yorkshire-pudding commented 5 months ago

Isn't it possible that two different modules could each have a submodule with the same name?

I.e project_a:submodule and project_b:submodule

klonos commented 5 months ago

I think that there are at least two scenarios that this format is trying to help with:

  1. What @yorkshire-pudding said, with regards to namespacing submodules that happen to have the same name, but belong to different parent modules.
  2. The case where you need to enable a submodule that is simply contained within a parent project, while at the same time not enabling the parent module (not-so-common use case, since most submodules depend on their parent module, still possible though - I think that ubercart is one such project)
klonos commented 5 months ago

...I think that ubercart is one such project

Nope, that's not it. I can't recall which project it was that had "independent" submodules in it 🤔

yorkshire-pudding commented 5 months ago

2. The case where you need to enable a submodule that is simply contained within a parent project, while at the same time not enabling the parent module (not-so-common use case, since most submodules depend on their parent module, still possible though - I think that ubercart is one such project)

I think it may also be that you know you need the submodule which may depends on the parent module (I also can't think of any actual modules that don't; though the examples module has this, it is not a real world example), but it is simpler (to write and to read) just:

dependencies[] = project:submodule

rather than

dependencies[] = project
dependencies[] = project:submodule
avpaderno commented 5 months ago

Isn't it possible that two different modules could each have a submodule with the same name?

No, because once the module files are loaded, and Backdrop core does load them when it completes its bootstrap phase, PHP would complain with an error similar to the following one.

PHP Fatal error: Cannot redeclare submodule_test() (previously declared in project_a/modules/submodule/submodule.module:8) in project_b/modules/submodule/submodule.module:12

yorkshire-pudding commented 5 months ago

Thanks @kiamlaluno - I forgot about that check. However that is functions rather than modules per se. In theory, if these submodules were using different sets of hooks, and had slight variances in naming methods, then this would not be prevented. I think this is a theoretical edge case, but I think it would be possible.

klonos commented 5 months ago

We discussed this problem during the dev meeting earlier today. I'd like to tast my theory and take a stab at fixing it.

klonos commented 5 months ago

Sorry for the noise. I'm trying to give this issue a meaningful title.

klonos commented 5 months ago

@stpaultim PR ready for testing here: https://github.com/backdrop/backdrop/pull/4802

The PR sandbox cannot be created, because we've reached our Tugboat quota. ...but you wouldn't be able to test this there anyway, since installing projects in the PR sandboxes has been broken for a while now.

klonos commented 5 months ago

...forgot to update here: the PR sandbox has been functional for some hours now.

stpaultim commented 4 months ago

@klonos Thanks very much for this PR. I will be pushing for some kind of solution to this problem.

However, bad news....

I just tried to install the Digital Agency Recipe on a local dev site with this PR and this time I actually get an error that prevents me from completing the install process.

This time, instead of failing to "fetch" the modules, it fails to install them, because they are already installed and forces me to abandon the installation and enabling process, because it's unable to do something that it already did.

For this particular workflow, it's a step backwards. The current/previous condition allows me to finish installing and enabling the modules as I'd like, it just throws and unnecessary warning. With this PR it actually prevents me from completing what I am trying to complete.

image

klonos commented 4 months ago

This time, instead of failing to "fetch" the modules, it fails to install them, because they are already installed and forces me to abandon the installation and enabling process, because it's unable to do something that it already did.

Thanks for testing @stpaultim 🙏🏼 ...I'll work on fixing that problem.

stpaultim commented 4 months ago

@klonos Thanks for your work on this issue.

I noticed that you had updated the PR, without asking for additional testing. But, I went ahead anyway and tested it again. Current status is the same as last time I tested it. But, you probably knew that already.