Implement new feature to build manifest from any composer.lock #16
Conversation
There was a problem hiding this comment.
I generally like the idea to use the composer.lock (and potentially composer.json) as an alternative source of information.
That being said, I'm not yet sure I understand the reasoning behind your overall approach. You created a class ManifestJsonDocument that to me is a mapper rather than a document, given it only returns ManifestDocument instances and does a lot of mapping of data into XML.
After that, you use our mapper to get that XML mapped to the Manifest object we actually want.
Why the intermediate format? That's redundant mapping involving the XML representation only to have it mapped to the actual Manifest Object in the next step. Why not directly map the json to our Manifest ?
If everything went well, our Serializer should be very much capable to map that into XML.
I'm also not convinced the ::fromComposer method belongs into the ManifestLoader as the idea here is logically coupled with loading our manifest from an xml format.
So maybe the composer handling deserves its own Loader? Maybe we want to call it ComposerConverter or ComposerImporter? That class could load what is needed (see other review comments) and pass that on to the mapper as JsonDocument(s) and get a Manifest-Object back. If you want to store that as XML then, that's up to the user.
Does that make any sense to you?
| $this->authors = new AuthorCollection(); | ||
| $this->authors->add(new Author('John Doe', new Email('[email protected]'))); | ||
| $this->license = 'MIT'; |
There was a problem hiding this comment.
I do understand the composer.lock does not contain this but usually the composer.json does.
So maybe we should change the implementation to use both files?
That would probably mean we should require a path as input to the ManifestLoader::fromComposer and not the actual filename.
There was a problem hiding this comment.
The above comment uses the "old" class names. The name will be different if the introduction comment would be addressed.
| { | ||
| $this->id = (string) ($decodedData['content-hash'] ?? $defaultId); | ||
| $this->name = (string) ($decodedData['name'] ?? 'vendor/package'); | ||
| $this->version = (string) ($decodedData['version'] ?? '0.0.0-dev'); |
There was a problem hiding this comment.
If I understand this correctly, this value will only be set when it (originally?) was set in the composer.json. But it is recommended to omit it as the version should be determined from the VCS.
Are we sure this is the best we can do here? I certainly don't know ;)
| $typeAttr = $doc->createAttribute('type'); | ||
| $typeAttr->value = $composerDoc->getType(); |
There was a problem hiding this comment.
Does this actually map 1:1? I wasn't aware we were that close ;)
|
First, I would like to thank you to take time for your review and comments !
With latest LTS Composer version 2.2 announcement, I'd like to use the Composer Runtime API with Installed versions component, and of course the
As I said in feature report, "to avoid rewrites too much code, I've reused lot of existing architecture as much as possible". Preambule: Keep the same way, whatever you handle, an XML string, XML file, contained into a PHAR, or from another source.
As
See my goal in preambule.
In this first version I didn't want to write a lot of code, or change current API, that works fine for all users. Except perharps for those, like me, that don't want to spent time to write an XML document (with possible XML syntax error, as it was the case with To conclude: I'm not convinced that this version is the best one, but it's a compromise ! In a better world, I'd like an API version that will be able to :
Examples: PHPUnit manifestPHP CompatInfo manifestNOTE: When you said "But it is recommended to omit it as the version should be determined from the VCS.", But it suffer that For example with this project and this PR, if we pick version from VCS, we may have something like
Waiting to hear you again, and clarify some points if necessary ! |
|
@theseer Working from a new draft (major API rewrites) ... Will propose a new PR tomorrow morning |
|
Looking forward to it. I won't have much time until the weekend to look at things or even reply to your above comments though... |
Understood. And I won't have time to finish it this morning, It will let me more hours to polish the new PR ;-) |
Thank YOU for contributing. That's the least a project owner can do...
Interesting option, but nothing I'd like to see in this repository. This repository is intended to be used as a dependency when ever someone wants to read a manifest.xml file - usually from within a phar archive. Strictly speaking, even the XML-Serializer should be moved out - but that's just one class and I considered that overkill. But we won't grow this to a gigantic mess of dependency hell and bloat that can do everything and nothing. That being said: We maybe should create additional repositories where people wanting to build based on composer data - may it be via API or by parsing the files - can simply use it.
Again, I fail to see where the need of a rewrite comes from?
No you don't. That's just superfluous. Having a
I still disagree with the approach ;)
I feel we're still not talking about the same thing here.
There is nothing to be done here. The phar stream wrapper does everything.
I fail to see what this is but given it's checked it's probably not too much of a problem ;)
PHPUnit's manifest is a very bad example because it's not based on
Interesting problem. I have to admit I didn't think much about yet. |
|
In summary, if I'll well understood :
That will conclude here my contribution, for this project. Thanks again to take time to reply to all my posts ! |
Not exactly what I meant even though I see one could have interpreted it like that. It depends on how you define "project" here. The focused on usecase for this Everything above that is out of scope and - as mentioned - that would include the XMLSerializer as that's the opposite direction. That - and I tried to point that out various times in my reponses - does not mean, I disagree with your idea of having something to create the same object structure from a different input format. It would merely require a new project (repository), for instance
I think this is the core of your/our misunderstanding:
Thanks for your time and efforts. I hope I managed to explain myself better and maybe you feel like contributing the idea as the aforementioned new project. If you're interested, I could create |
In such condition, an important example script is missing in your project, because I don't think all others projects that used your package generate a manifest like the following code for example :<?php
use PharIo\Manifest\ManifestSerializer;
require __DIR__ . '/../vendor/autoload.php';
$bundledComponentCollection = new \PharIo\Manifest\BundledComponentCollection();
$bundledComponentCollection->add(
new \PharIo\Manifest\BundledComponent(
'vendor/packageA',
new \PharIo\Version\Version('0.0.0-dev')
)
);
$manifest = new PharIo\Manifest\Manifest(
new \PharIo\Manifest\ApplicationName('vendor/package'),
new \PharIo\Version\Version('1.0.0'),
new \PharIo\Manifest\Library(),
new \PharIo\Manifest\CopyrightInformation(
new \PharIo\Manifest\AuthorCollection(),
new \PharIo\Manifest\License(
'BSD-3-Clause',
new \PharIo\Manifest\Url('https://spdx.org/licenses/BSD-3-Clause.html')
)
),
new \PharIo\Manifest\RequirementCollection(),
$bundledComponentCollection
);
echo (new ManifestSerializer)->serializeToString($manifest);
/*
* Output produced
*
<?xml version="1.0" encoding="UTF-8"?>
<phar xmlns="https://phar.io/xml/manifest/1.0">
<contains name="vendor/package" version="1.0.0" type="library"/>
<copyright>
<license type="BSD-3-Clause" url="https://spdx.org/licenses/BSD-3-Clause.html"/>
</copyright>
<requires>
<php version="*"/>
</requires>
<bundles>
<component name="vendor/packageA" version="0.0.0-dev"/>
</bundles>
</phar>
*/
👍
Why not, but at least a code base (to avoid to rewrite the same logic on each new mapper repository) should be available and independant from target generation. That is not currently the case with all
Yes you did !
Only if you are agreed to rework code base and make it independant from any mapper (see my comment above)
Sorry, but I'm not interrested with current code base. |
You do have a Point here. The documentation as well as examples are spares. Probably too sparse to be useful beyond the basic reading part. Your example is of course correct and that would be the intended way of use when (manually) generating a manifest.
You lost me here again, I'm afraid. Or I still fail to make myself clear. Let me try again. All the We could certainly argue that this repository - as small as it may be - could be split into 3:
With such a setup, you could combine as needed: In most cases, people would need the XML reading package - which would require the manifest classes - to be able to deal with the manifests in PHARs. If a project's task contains the need of creating a PHAR and wanting to add a Manifest, the Manifest Classes plus the XML Serializer are required. Now if the creating should be implemented based on composer provided input and we want to share such an implementation, we'd need yet another package that contains this fragment using the Manifest Classs pretty much like in your example and the XML Serializer to get it into our official Format. Now arguably all these by now 4 projects are closely related but I seriously dislike having dependencies on things I don't need or code coming along I don't want. So if we'd combine all these into one, we suddenly have the composer api package (or the symfony serializer) as a dependency just to be able to read the manifest.xml from a phar without ever wanting to write anything or create our own manifest. That's bad design (imho) and what I call bloat.
Not sure what you mean by "any mapper" but the Manifest Structure is independent. |
|
Forget all I said, its no more usefull. |
|
@theseer Thanks to had listen to me, and your patience. |
Implement feature describe in #15