Improve target-dir restriction for detecting new android project…

[Jule-]

…structure used in plugin.xml. (#575)

What does this PR do?

It just checks that source-file target-dir starts with app/ more than app in order to detect the new android project structure usage in plugin.xml.
All explanations in #575.

[brody4hire]

Thanks @Jule- for the contribution. I think you are right that we need to handle the case where target-dir with character other than / after app. But I think we are missing the answers to the questions I asked.

While reviewing the code related to this contribution I discovered another fix needed, raised in PR #577.

I think we should consider throwing an exception, emitting an error, and aborting the cordova plugin add command with an error status in case of target-dir that does not start with a valid prefix.

P.S. I just raised #578 to report error handling that may be needed.

[brody4hire]

Questions need to be resolved before we merge this one. I am entering this as a formal review to avoid a premature merge.

[Jule-]

Indeed you've found same issues as me... Sorry for the delay and parallel answer...

[brody4hire]

@Jule- I am not sure what you mean here.

As I said in #578 (comment): if you want to propose and discuss more changes to the getInstallDestination function I suggest you raise a new PR for discussion and review.

In general I would like to keep the number of "old fashioned" directory path transformations to a minimum, no more than required to avoid breaking old plugins for a while. "Old fashioned" plugins would generally have target-dir starting with src or libs. Plugin authors should have the freedom to put artifacts wherever they want into app. I also raised #580 to deprecate the "old fashioned" directory path transformations, to be removed in some future major release.

[Jule-]

Maybe some rewrites too for this PR.

[Jule-]

@brodybits tell me what do you think about this in order to patch the 7.1.x branch and then I could make a new PR for the master branche.

[brody4hire]

A couple things still needed:

• Test cases that demonstrate the cases that are resolved by these changes (I think they will be rare corner cases)
• Please rebase on master branch. We generally make fixes on master branch first then cherry-pick if we need to make a patch release.
• More succinct title

Some general comments:

I have some mixed feelings about this proposal.

First I would like to say that I am happy with the proposed changes in general. The proposed changes seem to improve the quality of the code and fix a couple corner cases.

My one doubt is that the need for these changes is still not super clear to me. Reviewing and testing these changes would divert some resources away from a number of much more important tasks that need our attention right now. This is especially true now that we want to deprecate and remove the old directory path support in #580, as I already said in #576 (comment).

So the benefit I see at this point is that the cleaner code would help us in case we encounter any more deprecated target directory issues in the future. I don't expect much more of an issue at this point but we never know.

I am also not convinced we should include this proposal on 7.1.x, unless we find another issue that affects existing plugins in the wild.

I would also like to get a review from one more member before merging. You may want to request another review from dev@cordova.apache.org, would suggest you solve these issues first and make a stronger case for this proposal.

[Jule-]

My one doubt is that the need for these changes is still not super clear to me. Reviewing and testing these changes would divert some resources away from a number of much more important tasks that need our attention right now.

It is blocking for me and I think for everyone who will try to get last cordova-android release 7.1.x and use plugins from https://github.com/katzer who is an involved Cordova plugin developer. His plugins are declared in appPlant... And he addressed a huge pain for starting apps in Cordova: native notifications.
All plugin maintainers like @katzer or @j3k0 are concerned about handling Cordova versions for the long term and last release break some plugins for a wrong reason (bad path checking). I try to do my best for an seamless PR. I don't think #589 is a big deal and there is a lot of tests on it.
Thanks for your time and concern.

[janpio]

It is blocking for me and I think for everyone who will try to get last cordova-android release 7.1.x and use plugins from @katzer who is an involved Cordova plugin developer. His plugins are declared in appPlant...

That should have been fixed with 7.1.4 which was released quickly after this bug was discovered.