minor improvement for inserting comments

Discussion corner for Developers of Serendipity.
Post Reply
Anson
Regular
Posts: 24
Joined: Thu Apr 16, 2009 7:05 am

minor improvement for inserting comments

Post by Anson »

I recently created a new s9y blog, with data imported from a LiveJournal account. I ended up writing a custom importer that I hope to share soon (there are still some loose ends to clean up), but the gist is that instead of using the XML files directly from LiveJournal.com I used the XML files generated by jbackup.pl, which is slightly different (and more importantly, includes data about comments).

However, I ran into an issue where I couldn't easily find out the ID of the comment that I'd just inserted, necessary for importing comments in their threaded state. Adding

Code: Select all

return $cid;
as line 896 of include/functions_comments.inc.php (the last line of serendipity_insertComment) seems like the correct fix. Anything expecting a TRUE return from that function will get a value that evaluates to truth, and anything wanting the actual comment ID for whatever reason gets it.

As for the jbackup importer, I basically took the existing LJ importer, added support for comments and changed the name of some expected tags to match the output of jbackup. It doesn't yet support importing the mood or tags of entries (which would require checking whether the appropriate plugin is installed first, that's what I'm looking at), and it doesn't handle comments posted by OpenID-authenticated users either (they show up as users named "ext_NNNN").
garvinhicking
Core Developer
Posts: 30022
Joined: Tue Sep 16, 2003 9:45 pm
Location: Cologne, Germany
Contact:

Re: minor improvement for inserting comments

Post by garvinhicking »

Hi!

That sounds good! Indeed returning the ID inside the functionw ould be helpful, so I've patched that as well for the next version.

Would you maybe be able to adapt the importer in a way, that the user could specify through a boolean option whether to use default XML output or the jbackup one? Then we could have it all inside a single importer module...

Regards,
Garvin
# Garvin Hicking (s9y Developer)
# Did I help you? Consider making me happy: http://wishes.garv.in/
# or use my PayPal account "paypal {at} supergarv (dot) de"
# My "other" hobby: http://flickr.garv.in/
Anson
Regular
Posts: 24
Joined: Thu Apr 16, 2009 7:05 am

Re: minor improvement for inserting comments

Post by Anson »

garvinhicking wrote:That sounds good! Indeed returning the ID inside the functionw ould be helpful, so I've patched that as well for the next version.
Thanks! Aside from thinking it might be a good general improvement to the code, I mention it so that after the next release I don't have spurious errors when I validate the installation's integrity. :)
Would you maybe be able to adapt the importer in a way, that the user could specify through a boolean option whether to use default XML output or the jbackup one? Then we could have it all inside a single importer module...
Yeah, that shouldn't be too hard. I mean, I started off by just making a few changes to the included importer, and separated it out to (again) deal with complaints validating the installation. Incorporating it all into one module would just involve accepting multiple tags to set the timestamp of an entry, and supporting comments on an entry if they exist. No user configuration would be necessary in that case, and the only problem left to solve is handling OpenID commenters.

It would be a bit hairier to handle importing comments from jbackup's output and the stock export_comments.bml: jbackup denormalizes the output of export_comments so each entry has the name of the person commenting, and export_comments.bml uses IDs for each comment and lets you query metadata separately to associate those IDs with usernames. On the other hand, there are a lot of problems like that with the stock output of LiveJournal's exporters: posts are exported on a per-month basis I believe, comments have to be exported separately, metadata on comments (and maybe posts, not sure) has to be exported separately... migrating a blog that has had any kind of lifespan is just incredibly more simple using jbackup. The closer the LJ importer gets to handling all of that correctly and with distinct XML files (or querying LiveJournal directly!), the closer it gets to being a PHP reimplementation of jbackup.pl, which is itself 1,039 lines of Perl.

I'm not really interested in doing that, so unless someone else wants to do it the importer will have essentially two modes: import the posts, minus comments, from any given run of LJ's "Export Journal" tool, OR import all posts and comments ever, by grabbing the output of jbackup.pl.
Anson
Regular
Posts: 24
Joined: Thu Apr 16, 2009 7:05 am

Re: minor improvement for inserting comments

Post by Anson »

OK... for now I'm skipping integration with moods and freetag; and I'm not sure what to do about OpenID comments (on the bright side, they're pretty uncommon) without handling it on a case-by-case basis. I've added one configuration option, the LJ username, which is only used if there are comments in the XML - it replaces any instance of the supplied LJ username with the real name of the current s9y user, and uses the URL of the s9y installation as the URL.

Would you like me to just paste in the unified diff? Email it?
garvinhicking
Core Developer
Posts: 30022
Joined: Tue Sep 16, 2003 9:45 pm
Location: Cologne, Germany
Contact:

Re: minor improvement for inserting comments

Post by garvinhicking »

Hi!

That's great, I'm happy to look at it. If you can attach the diff or complete file simply to this forum?

Regards,
Garvin
# Garvin Hicking (s9y Developer)
# Did I help you? Consider making me happy: http://wishes.garv.in/
# or use my PayPal account "paypal {at} supergarv (dot) de"
# My "other" hobby: http://flickr.garv.in/
Anson
Regular
Posts: 24
Joined: Thu Apr 16, 2009 7:05 am

Re: minor improvement for inserting comments

Post by Anson »

Attached.
Attachments
ljdiff.txt
(5.21 KiB) Downloaded 373 times
garvinhicking
Core Developer
Posts: 30022
Joined: Tue Sep 16, 2003 9:45 pm
Location: Cologne, Germany
Contact:

Re: minor improvement for inserting comments

Post by garvinhicking »

Hi!

Great! I'm away for the weekend, so please hang on til next week for feedback :)

Regards,
Garvin
# Garvin Hicking (s9y Developer)
# Did I help you? Consider making me happy: http://wishes.garv.in/
# or use my PayPal account "paypal {at} supergarv (dot) de"
# My "other" hobby: http://flickr.garv.in/
Anson
Regular
Posts: 24
Joined: Thu Apr 16, 2009 7:05 am

Re: minor improvement for inserting comments

Post by Anson »

Will do... I might have something else cooked up by then too. :-P
garvinhicking
Core Developer
Posts: 30022
Joined: Tue Sep 16, 2003 9:45 pm
Location: Cologne, Germany
Contact:

Re: minor improvement for inserting comments

Post by garvinhicking »

Hi!

Great, I've just committed your patch. It looks very good to me; even though I did not have a livejournal dump to test it with, I don't see a reason why the code shouldn't work. :)

Many thanks for contributing this,
Garvin
# Garvin Hicking (s9y Developer)
# Did I help you? Consider making me happy: http://wishes.garv.in/
# or use my PayPal account "paypal {at} supergarv (dot) de"
# My "other" hobby: http://flickr.garv.in/
Anson
Regular
Posts: 24
Joined: Thu Apr 16, 2009 7:05 am

Re: minor improvement for inserting comments

Post by Anson »

Great!

Here are the other things I'm working on, in case I'm duplicating effort:
  • an HTML Purifier markup plugin, so that I can allow safe HTML in comments rather than requiring some other markup language like BBcode or what-have-you.
  • related to that, I've noticed that a lot of markup functionality is repeated across all the markup plugins ($this->markup_elements, the extended properties' disabling of individual markup plugins per post, that kind of thing), I'm thinking about writing a serendipity_event_markup_plugin class from which markup plugins can inherit.
  • I've also noticed that there are still places where pretty URLs aren't used, and I'm trying to weed them out. Some of this involves the URLs generated by my theme (bulletproof), most of it relates to comments (show threaded/show linear links, the URL to which comments are posted, etc.)... this is the area where I know the least, I'm just trying to make changes and then test them with/without mod_rewrite enabled.
garvinhicking
Core Developer
Posts: 30022
Joined: Tue Sep 16, 2003 9:45 pm
Location: Cologne, Germany
Contact:

Re: minor improvement for inserting comments

Post by garvinhicking »

Hi!
[*] an HTML Purifier markup plugin, so that I can allow safe HTML in comments rather than requiring some other markup language like BBcode or what-have-you.
Maybe you'd like to integrate this into the serendipity_event_unstrip_tags event plugin; this already works on at least showing escaped HTML in the comment, so you might be able to use that as a base to offer people restrictive HTML parsing.
[*] related to that, I've noticed that a lot of markup functionality is repeated across all the markup plugins ($this->markup_elements, the extended properties' disabling of individual markup plugins per post, that kind of thing), I'm thinking about writing a serendipity_event_markup_plugin class from which markup plugins can inherit.
The basic problem of this is that it's kind of an API change, and instead people being able to install a single plugin and letting it work on that, is that you'd create another dependency. I agree that the code is a bit redundant, but on the other hand it's onle small 4-5 lines of this foreach/if routine.

It might also create trouble for people that already use one plugin, and I'm not sure if the s9y core already supports installing dependencies, when the dependency did not initially exist in the installation phase of the plugin.
[*] I've also noticed that there are still places where pretty URLs aren't used, and I'm trying to weed them out. Some of this involves the URLs generated by my theme (bulletproof), most of it relates to comments (show threaded/show linear links, the URL to which comments are posted, etc.)... this is the area where I know the least, I'm just trying to make changes and then test them with/without mod_rewrite enabled.[/list]
Yeah, most of these links are used due to url rewriting, and that for Apache using the ErrorDocument rewriting method, you cannot pass GET/POST parameters to a rewritten URL, so the pages would not see the parameter. So it does not only depend whether you use url rewriting or not, but also if you use ErrorDocument or mod_rewrite.

For the sake of not forking the code and making it work in all three cases, on situations where variable appending is required, s9y always uses the non-rewrite URL.


Best regards,
Garvin
# Garvin Hicking (s9y Developer)
# Did I help you? Consider making me happy: http://wishes.garv.in/
# or use my PayPal account "paypal {at} supergarv (dot) de"
# My "other" hobby: http://flickr.garv.in/
Anson
Regular
Posts: 24
Joined: Thu Apr 16, 2009 7:05 am

Re: minor improvement for inserting comments

Post by Anson »

garvinhicking wrote:Maybe you'd like to integrate this into the serendipity_event_unstrip_tags event plugin; this already works on at least showing escaped HTML in the comment, so you might be able to use that as a base to offer people restrictive HTML parsing.
Well, I based it on unstrip_tags, that's how I figured out how to get at the original text of the submitted comment. For now I'm just testing it, when I'm happy I'll see whether it would make sense to put it in serendipity_event_unstrip_tags as a different kind of "unstrip" functionality.
The basic problem of this is that it's kind of an API change, and instead people being able to install a single plugin and letting it work on that, is that you'd create another dependency. I agree that the code is a bit redundant, but on the other hand it's onle small 4-5 lines of this foreach/if routine.
That's true, it would be an API change that either adds a dependency or breaks backward compatibility of existing plugins. On the other hand, I think it's a bit more than 4-5 lines:

Code: Select all

        $this->markup_elements = array(
            array(
              'name'     => 'ENTRY_BODY',
              'element'  => 'body',
            ),
            array(
              'name'     => 'EXTENDED_BODY',
              'element'  => 'extended',
            ),
            array(
              'name'     => 'COMMENT',
              'element'  => 'comment',
            ),
            array(
              'name'     => 'HTML_NUGGET',
              'element'  => 'html_nugget',
            )
        );

        $conf_array = array();
        // Add markup elements config
        foreach($this->markup_elements as $element) {
            $conf_array[] = $element['name'];
        }
        $propbag->add('configuration', $conf_array);
...which is required for serendipity_event_entryproperties to know a particular plugin is a markup plugin that can be disabled on a per-post basis (as well as to provide the standard "where to apply this markup" configuration options), plus the following

Code: Select all

                case 'frontend_display':
                    foreach ($this->markup_elements as $temp) {
                        if (serendipity_db_bool($this->get_config($temp['name'], true)) && isset($eventData[$temp['element']]) &&
                            !$eventData['properties']['ep_disable_markup_' . $this->instance] &&
...to actually implement it.
It might also create trouble for people that already use one plugin, and I'm not sure if the s9y core already supports installing dependencies, when the dependency did not initially exist in the installation phase of the plugin.
Well that part could be handled by upping the required version of s9y, and making it part of the API instead of another plugin... but I had not considered API stability, and I can see how that's kind of an issue. Maybe I'll bring it up again if you start thinking about a flag day (like s9y 2.0). :)
Yeah, most of these links are used due to url rewriting, and that for Apache using the ErrorDocument rewriting method, you cannot pass GET/POST parameters to a rewritten URL, so the pages would not see the parameter. So it does not only depend whether you use url rewriting or not, but also if you use ErrorDocument or mod_rewrite.
Ah, okay. I had not particularly investigated the ErrorDocument form of rewriting.
For the sake of not forking the code and making it work in all three cases, on situations where variable appending is required, s9y always uses the non-rewrite URL.
Hmmmm. I'm going to keep looking at it, I really want clean URLs throughout.
Post Reply