This will be more compatible with other modules that use the same library.

CommentFileSizeAuthor
#7 activitystream-simplepie-library-6x.2x.1206466.patch362.2 KBlyricnz
#5 activitystream-simplepie-library-6x.2x.1206466.patch362.24 KBlyricnz
#4 activitystream_feed.install.txt1.87 KBlyricnz
#3 activitystream-simplepie-library-1206466.patch360.13 KBlyricnz

Comments

lyricnz’s picture

lyricnz commented
Category: task » bug
Priority: Normal » Major

NOTE: Activity Stream may NOT include simplepie.inc in the drupal.org repository - since it's not GPL. Even though Simplepie is licensed under a more permissive license - see http://drupal.org/licensing/faq

lyricnz’s picture

lyricnz commented
Title: Port activitystream use of SimplePie to use Libaries module » Activity Stream includes non-GPL code (simplepie)
lyricnz’s picture

lyricnz commented
Version: 6.x-2.x-dev » 6.x-1.x-dev
StatusFileSize
new activitystream-simplepie-library-1206466.patch360.13 KB

Here's a patch for 6.x-1.x that:
- updates INSTALL.txt to mention that Libraries and SimplePie are required
- adds libraries as dependency for activitystream_feed
- moves hook_requirements from activitystream_feed.module to activitystream_feed.install (bug)
- checks for libraries and simplepie in hook_requirements, "tidies up" messages a bit (YMMV)
- changes activitystream_feed to use the library
- remove a few trailing whitespace
- remove simplepie.inc (the bulk of this patch)

lyricnz’s picture

lyricnz commented
StatusFileSize
new activitystream_feed.install.txt1.87 KB

And the missing .install file.

lyricnz’s picture

lyricnz commented
Version: 6.x-1.x-dev » 6.x-2.x-dev
Status: Active » Needs review
StatusFileSize
new activitystream-simplepie-library-6x.2x.1206466.patch362.24 KB

And a patch for 6.x-2.x. Same as above, except:
- skip EOL whitespace changes
- undo the module-weight kludge in activitystream.install, fix phpdoc
- check for simplepie properly in activitystream_feed_streamapi() and emit error if not present

xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
commented
Status: Needs review » Needs work

Thanks for this. I started porting this patch to match the 7.x branch as well, but didn't finish it last night.

One issue I noticed was that the missing simplepie error directs people to INSTALL.txt, and the instructions are in README.txt.

lyricnz’s picture

lyricnz commented
Status: Needs work » Needs review
StatusFileSize
new activitystream-simplepie-library-6x.2x.1206466.patch362.2 KB

Attached

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
commented
Priority: Major » Normal
Status: Needs review » Reviewed & tested by the community

Technically, simplepie can be re-licensed as GPL2 as we have done in core, so I'm going to bump this back to normal.

+    else {
+      // Libraries module not found - perhaps due to install profile. Relax.
+    }

You can probably get rid of that else. It'll save a few microseconds.

Other than that, this patch looks good.

akalsey’s picture

akalsey commented
Status: Reviewed & tested by the community » Needs work

While I appreciate the effort put into a patch, I'm going to reject it like I reject all the similar patches that come every so often. Not only is simple pie available as gpl, making this moot, but I'm not inclined to make the module harder to install. Drupal modules are hard enough to install for the average user already, without adding external dependancies they need to hunt down and install.

Some of the other changes look good, and I'd love to see a patch containing only those.

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
commented

Can you define "some of the other changes" please?