3rd party libraries in marketplace packages
Permalink
I'd like to ask whether we could get some common conventions / rules over the 3rd party libraries in marketplace packages. If I remember correctly, in 5.6 marketplace the whole /libraries/3rdparty was excluded from the automated case.
This does not seem to be the case for 5.7, though. The whole /vendor directory is part of the automated tests which makes a lot of them fail because we cannot obviously control all the 3rd party vendors providing these libraries we rely on.
We have used a method similar to Korvin's suggestion in including these libraries through composer in our package:
https://github.com/Buttress/addon_composer_sample/...
It was actually suggested here as a way to handle these in packages:
http://www.concrete5.org/community/forums/5-7-discussion/3rd-party-...
So, any suggestions on how to handle this properly? Could we maybe discuss about excluding these from the automated tests? Or maybe build some logic to the marketplace submission process that would automatically download the dependencies (based on the composer.lock file) and include them in the submitted package. This way, the marketplace would kind of add a layer of security into the submission process, although this wouldn't give any guarantee that none of the packages wouldn't e.g. contain the "eval" function.
This does not seem to be the case for 5.7, though. The whole /vendor directory is part of the automated tests which makes a lot of them fail because we cannot obviously control all the 3rd party vendors providing these libraries we rely on.
We have used a method similar to Korvin's suggestion in including these libraries through composer in our package:
https://github.com/Buttress/addon_composer_sample/...
It was actually suggested here as a way to handle these in packages:
http://www.concrete5.org/community/forums/5-7-discussion/3rd-party-...
So, any suggestions on how to handle this properly? Could we maybe discuss about excluding these from the automated tests? Or maybe build some logic to the marketplace submission process that would automatically download the dependencies (based on the composer.lock file) and include them in the submitted package. This way, the marketplace would kind of add a layer of security into the submission process, although this wouldn't give any guarantee that none of the packages wouldn't e.g. contain the "eval" function.
OK, so I guess we just have to handle this in the PRB review process then.
That's not what Korvin (from the core team) suggests in the linked example, though.
Well, that's kind of a bummer that we need to basically go through a large amount of 3rd party code to get our add-on approved. Do you want us to write a book to explain how all the Symfony components work internally? ;) Well, I think there might be those books available already by some other authors.
E.g. currently our submission fails the eval() test because it includes a Symfony component (symfony/twig-bridge) that then internally requires the symfony/expression-language component which comes with this function. And to understand why it uses the eval() method, you as a PRB member would need to understand what that package does. Or is it really on our shoulders to give you a full lecture on how all the components in the Symfony libraries work? This might make the approval process somewhat clumsy and long overall...
Yeah, and I think most composer libraries include the license, we only include libraries with a license that is compatible with ours.
Ideally, it still helps us if they are located in a /3rdparty/ directory,
That's not what Korvin (from the core team) suggests in the linked example, though.
Where they fail automated tests, you need to post explaining why.
Well, that's kind of a bummer that we need to basically go through a large amount of 3rd party code to get our add-on approved. Do you want us to write a book to explain how all the Symfony components work internally? ;) Well, I think there might be those books available already by some other authors.
E.g. currently our submission fails the eval() test because it includes a Symfony component (symfony/twig-bridge) that then internally requires the symfony/expression-language component which comes with this function. And to understand why it uses the eval() method, you as a PRB member would need to understand what that package does. Or is it really on our shoulders to give you a full lecture on how all the components in the Symfony libraries work? This might make the approval process somewhat clumsy and long overall...
Yeah, and I think most composer libraries include the license, we only include libraries with a license that is compatible with ours.
And also I've noticed that some of these fails are because of the test cases that come with these packages and they using some forbidden method for instance.
Clarifications.
- it would not be directly beneath the package's main directory, but somewhere beneath /src/ or /vendor/.
- if beneath vendor, then we already know its 3rd party, so doesn't matter
- if beneath src and its 3rd party, then we need to be told that it is, simply placing /3rdparty/ in the path makes that obvious, but you can use other ways to tell us.
- On failed linter tests, a simple quick note is all that is needed giving us enough info to justify the exception. Just enough to make it clear what/why. Any more and you waste your time writing it and our time reading it. If a failure is in bundled test code and not in code that is used, all you need to do is post a note saying so.
I would welcome more automation of the automated checks in a way that would identify and exempt 3rd party code , but for that you need to convince the core team as Korvin is the man who looks after that functionality.
In the mean time, the number 1 rule of getting stuff approved remains 'Make the reviewers job easy'. Anything developers do to make a submission easier to review means that it will be approved sooner.
If we have to wade through a megabyte of 3rd party code a submission won't be approved that quickly. If we know it is third party code and hence uses prescribed functions to do X and Y, and code doing Z is never actually used, then we can make a quick check, exempt that megabyte, and get on with reviewing the rest of the submission.
- it would not be directly beneath the package's main directory, but somewhere beneath /src/ or /vendor/.
- if beneath vendor, then we already know its 3rd party, so doesn't matter
- if beneath src and its 3rd party, then we need to be told that it is, simply placing /3rdparty/ in the path makes that obvious, but you can use other ways to tell us.
- On failed linter tests, a simple quick note is all that is needed giving us enough info to justify the exception. Just enough to make it clear what/why. Any more and you waste your time writing it and our time reading it. If a failure is in bundled test code and not in code that is used, all you need to do is post a note saying so.
I would welcome more automation of the automated checks in a way that would identify and exempt 3rd party code , but for that you need to convince the core team as Korvin is the man who looks after that functionality.
In the mean time, the number 1 rule of getting stuff approved remains 'Make the reviewers job easy'. Anything developers do to make a submission easier to review means that it will be approved sooner.
If we have to wade through a megabyte of 3rd party code a submission won't be approved that quickly. If we know it is third party code and hence uses prescribed functions to do X and Y, and code doing Z is never actually used, then we can make a quick check, exempt that megabyte, and get on with reviewing the rest of the submission.
On failed linter tests, a simple quick note is all that is needed giving us enough info to justify the exceptionOK, this is great information and sounds perfectly fine anyways.
Tomorrow, I'll go through all the failed tests individually and try to summarize their reasons to a few words. All I've done so far is to verify that they are all coming from the /vendor directory.
Thanks John!
Any submission needs to be complete, with all necessary libraries bundled with it. ie, concrete5.org or end users can't be expected to run composer to build it for you.
Ideally, it still helps us if they are located in a /3rdparty/ directory, or if that is not feasible, the directories need to be identified in a review post.
Where they fail automated tests, you need to post explaining why. We can then check and grant an exemption for that specific test.
Again ideally, we like only the 3rd party code that is actually used to be bundled with the submission. However, that isn't always practical with libraries pulled in with composer, so again, if there is a reasonable justification, then we can grant an exemption for extraneous or unused files.
Last point. For any 3rd part code we need details of the license that code is used under. This needs to be both posted to the review and included as either a comment in the relevant 3rd party files or an text file associated with them.