Prevent double download (to contrib folder) if module exists in main folder

[indigoxela]

This is something Bee shares with Drush.

When downloading a module, the existence of that module in the modules folder is checked, but if a contrib subdirectory exists, and the module exists in the main /modules directory, the module is downloaded again.

To reproduce:

• Create a contrib subdirectory in /modules
• Try to download a module you already have in /modules
• You'll end up with two copies of that module, one in /modules, one in /modules/contrib

The download is already prevented if no contrib subdir exists, or if the module already is in the contrib folder.

[yorkshire-pudding]

Interesting. I wonder how common the case is that people start downloading modules and then switch to doing contrib/custom without moving all the contrib ones into contrib.

[indigoxela]

I wonder how common the case is that people start downloading modules and then switch to doing contrib/custom...

I'm not sure, either. Possibly not that common, as drush (still) does the same. But if it were easy to catch...

[yorkshire-pudding]

Yes, it probably is relatively simple to catch. Needs a bit of thought to ensure the messages are accurate and make sense.

[yorkshire-pudding]

@indigoxela
I'm looking at this in conjunction with #259. What do you think we should do if the user tries to download a contrib module that exists in core? My instinct is that we shouldn't allow this but I'd like your views.

[indigoxela]

What do you think we should do if the user tries to download a contrib module that exists in core?

I can't think of a use-case for that. If something went into core, the desired functionality is available already. Downloading the obsolete module causes confusion and IMO provides no benefit. But that's just my 2c.

[yorkshire-pudding]

Hi @indigoxela - Sorry I've been quiet on this for so long. I've prepared a pull request.
If I download a project to the main folder, then create contrib/custom subfolders and try to download again, it will now give a message and not download.
For multisites:
If I download without the site specified then it goes into /modules. If I then try to download with site specified then it will give a message and not download
If I download first with the site specified then it goes into sites/site_name/modules. If I then try to download without site specified then it will download.
This behaviour seems correct.

I've also improved the messaging around downloading dependencies

[indigoxela]

@yorkshire-pudding many thanks for your work, will try to test ASAP. 👍

[ghost]

For multisites:
If I download without the site specified then it goes into /modules. If I then try to download with site specified then it will give a message and not download

That doesn't seem right... There are use-cases for having the same module in both the root modules directory and the site-specific modules directory. For example, a module is installed in root, but then for a particular site you need a downgraded or patched version.

So I'd prefer if that were reverted and this issue just focused on the root vs. root contrib issue.

[yorkshire-pudding]

@BWPanda - I think the use case you describe is an edge case, but a valid one nonetheless. However the use case you give couldn't currently be done with bee download; it would need #56 to be implemented.
However, I will add an option to --allow-multisite-copy (ideas for alternative wording welcome) which would allow to bypass this particular check.

[ghost]

However the use case you give couldn't currently be done with bee download;

I've done this by just downloading normally, then manually applying a patch from a PR, for example. So it is possible and I don't want to see any regressions in that regard.

[yorkshire-pudding]

I don't think that making a niche use case use an option to enable this so that for the majority of use cases it is simple and reduces duplication is a regression. In most cases, we focus on making it easier for the majority whilst making advanced and (some) niche uses possible, so I think this approach is in line with that.

[yorkshire-pudding]

@indigoxela and @BWPanda - new PR which adds an option to download an additional multisite copy of a project. Also fixes whitespace introduced in a5f2185.
Also includes some tweaks to the dependency handling to ensure we don't try to download dependencies twice.

[indigoxela]

@yorkshire-pudding oops, I promised to test, but forgot to do it (shame on me). Many thanks for your work and patience. 🙏

[yorkshire-pudding]

No problems. As Peter requested the ability to override for a multisite copy, I needed to do some extra work anyway.

[yorkshire-pudding]

@indigoxela - a gentle reminder

[ghost]

--allow-multisite-copy is quite long, and for people with multisites they'll likely be using this a lot... Can we shorten it to something like --force and then make that ignore all checks and just download anyway? There may be use cases for this outside of multisites.

[yorkshire-pudding]

I'm happy to rename with the same function. I can't think of any genuine use cases for this outside of multisite and it would require more rework than a simple rename.

[indigoxela]

... a gentle reminder

Yes, this time I do! 😏

This is how I tested after applying the patch in my bee install:

1. Enter a single install Backdrop
2. Ran bee dl somemodule for an existing one and got nagging that it exists 👍
3. Newly created a contrib subdir, ran command again - still get the nagging 👍

Switched to a multi-site install.

1. Enter one of the sites
2. Ran bee dl somemodule for an existing one - nagging 👍
3. Newly created a contrib dir, ran command again - still get the nagging 👍

Then the new param test.

1. Ran bee dl somemodule for one that exists in the main modules directory
2. Got nagging as expected 👍
3. Added the command line param --allow-multisite-copy
4. Download worked this time 👍

Probably I didn't test every case, but the problem I reported initially is gone. 🎉

[yorkshire-pudding]

Thanks @indigoxela - much appreciated

[yorkshire-pudding]

@BWPanda - looking at this again after a short break, I'm not convinced about changing this.
--force becomes vague and non-descriptive.
There really is no use case outside multisite as it will flag up duplicate modules on the status page.

I have a few multisite instances (for me and for some clients) and I really have not come across this need on any of them. I grant that it will be needed in some multisite cases, but I really can't see this being needed widely.

If there is a need for shorter option names, perhaps we should consider global functionality to allow options to have aliases like we do for commands? This is common in many command line tools where you can use --verbose-option or -v and they do the same thing.

[yorkshire-pudding]

Also, there is a precedent in other commands to use descriptive options that are longer than one word:

• --hide-progress
• --no-dependency-checking
• --show-password
• --no-clean-urls