backdrop-contrib / project

Projects associate a code-based project with releases and power the update server of BackdropCMS.org
2 stars 10 forks source link

Allow other modules to include data in the project XML files #25

Closed jlfranklin closed 5 years ago

jlfranklin commented 5 years ago

To enable module package signing (see backdrop/backdrop-issues#1992) without making it part of the primary project module, I'd like to add an alter hook to Project when writing out the XML. This will allow a separate module specific to the kind of signature (OpenSSL, GnuPG, Sodium) to add the digital signature tags and keep requirements checks for PHP crypto extensions out of Project.

This may be generally useful for other packages. For example, project_github could add links to the GitHub pages through such a hook, and project_usage could add popularity data.

quicksketch commented 5 years ago

The PR at https://github.com/backdrop-contrib/project/pull/26 looks amazing. Nice job! Can you confirm that the XML before and afterwards is exactly the same? I can do some checking with my localhost version of backdropcms.org.

jlfranklin commented 5 years ago

They should be congruent, but not exactly the same. Newlines and whitespace between tags may be added or removed, and the tag order slightly different. I'll see if I can come up with a couple tests in my dev VM.

quicksketch commented 5 years ago

Indentation is quite a bit different, but let's punt that over to a new issue I opened at https://github.com/backdrop/backdrop-issues/issues/3667. I have a PR in the works to make indentation work correctly, but it's just an aesthetic issue.

quicksketch commented 5 years ago

Looks like output is identical, but yeah in different tag order. This is minor, but could we update it to be in the exact same order? That may save us from causing unexpected test failures or needing to regenerate the test XML files where we do explicit string checking that may be dependent on order.

Here's a hacked-up diff showing the misordered items on a project feed:


 <project xmlns:dc="http://purl.org/dc/elements/1.1/">
   <title>Webform</title>
   <short_name>webform</short_name>
   <dc:creator>drop</dc:creator>
   <type>project_module</type>
   <api_version>1.x</api_version>
-  <project_status>published</project_status>
   <recommended_major>1</recommended_major>
   <supported_majors>1,4</supported_majors>
   <default_major>1</default_major>
+  <project_status>published</project_status>
   <link>http://local.backdropcms.org/project/webform</link>
   <releases>
     <release>
       <name>webform 1.x-4.17.0</name>
       <version>1.x-4.17.0</version>
-      <status>published</status>
-      <date>1527518524</date>
-      <filesize>283018</filesize>
-      <security_update>false</security_update>
       <version_major>4</version_major>
       <version_minor>17</version_minor>
       <version_patch>0</version_patch>
+      <status>published</status>
       <release_link>https://github.com/backdrop-contrib/webform/releases/tag/1.x-4.17.0</release_link>
       <download_link>https://github.com/backdrop-contrib/webform/releases/download/1.x-4.17.0/webform.zip</download_link>
+      <download_md5>725034bd50a7d99b365b380926cc9e7d</download_md5>
+      <download_signature></download_signature>
+      <date>1527518524</date>
+      <filesize>0</filesize>
+      <security_update>false</security_update>
     </release>
     <release>
       <name>webform 1.x-4.16.1</name>
       <version>1.x-4.16.1</version>
-      <status>published</status>
-      <date>1522201894</date>
-      <filesize>289776</filesize>
-      <security_update>false</security_update>
       <version_major>4</version_major>
       <version_minor>16</version_minor>
       <version_patch>1</version_patch>
+      <status>published</status>
       <release_link>https://github.com/backdrop-contrib/webform/releases/tag/1.x-4.16.1</release_link>
       <download_link>https://github.com/backdrop-contrib/webform/releases/download/1.x-4.16.1/webform.zip</download_link>
+      <download_md5></download_md5>
+      <download_signature></download_signature>
+      <date>1522201894</date>
+      <filesize>289776</filesize>
+      <security_update>false</security_update>
     </release>
   </releases>
 </project>

It's also clear that download_md5 and download_signature are added via this PR. Seems like download_md5 is actually real and working, but until we have download_signature actually doing something, should that be removed?

jlfranklin commented 5 years ago

I don't know where that's coming from. The strings "download_signature" and "download_md5" don't appear anywhere in my source tree, not as a variable names, not as a string literals, not as array indexes nor object properties.

quicksketch commented 5 years ago

Oh, sorry I had the diff backwards! I that must have been from a previous testing where I had been testing something. Your version actually has them removed.

And on further introspection, our tests are already formatted differently and just use mock files rather than real data from project module, so I don't think it's going to make any difference. And I like the new ordering better.

So I think this is good to go. If you could review https://github.com/backdrop/backdrop-issues/issues/3667 then we can get the formatting problem resolved at the same time.

quicksketch commented 5 years ago

I was about to merge this when I saw it didn't include any documentation. Could you add a new project_release.api.php file to the project_release module and document the new hooks?

jlfranklin commented 5 years ago

Sure. I should have time today to put that together.

jlfranklin commented 5 years ago

Now with documentation on the new hooks.

quicksketch commented 5 years ago

Thanks @jlfranklin! Finally merged https://github.com/backdrop-contrib/project/pull/26 into 1.x.