Project

General

Profile

Bug #2174

Featured Content Domain Mapping Support

Added by Dominic Giglio almost 9 years ago. Updated almost 9 years ago.

Status:
Resolved
Priority name:
Low
Category name:
WordPress (misc)
Target version:
Start date:
2012-10-08
Due date:
% Done:

0%

Estimated time:

Description

The CAC Featured Content Widget issue page on Github has received a request for interoperability with the Domain Mapping Plugin from WPMU Dev:

http://premium.wpmudev.org/project/domain-mapping

I'm opening this ticket for commons feedback on the request.

History

#1 Updated by Dominic Giglio almost 9 years ago

Boone,

As a new plugin dev supporting his first plugin, could you give me some advice on this? I started working on it but I find myself wondering if including support for a separate plugin in this plugin's code is something that's done often. I guess the domain mapping plugin is important to MS admins and the discussion on the Github issue does point out good reasons for including the support, but I don't want to put a bunch of work towards a new feature that you might know a better way of implementing.

Thanks

#2 Updated by Boone Gorges almost 9 years ago

Dom, in these cases, you've got to make a judgment call. There are two factors to consider: (1) How much work will it take to make the plugins interoperable? and (2) What's the potential impact of the interoperability? Re (1), you don't want to have to write piles of code to make the plugins work together, especially if it'll take weird hacks. Re (2), it's presumably more worthwhile to go to the effort if the plugin is really widely used.

As for this specific case. I downloaded the WPMUDev Domain Mapping plugin from the zip file linked on the Github ticket. I had a hunch that the plugin works by filtering URLs wherever they appear, and doing a str_replace() to swap the regular domain with the mapped domain. And it looks like this is more or less the case. That means that there's a very lightweight way to make your plugin compatible with the domain mapping plugin is to run your data through the same filter(s) that the plugin uses for this purpose.

More concretely, the DM plugin does the following (domain-mapping.php:233):

add_filter( 'home_url', array(&$this, 'swap_mapped_url'), 10, 3);

home_url is a WP core filter that's run wherever the blog's URL is requested from the options table (see the end of this function https://core.trac.wordpress.org/browser/tags/3.4.2/wp-includes/link-template.php#L1863). In your views (eg views/cac-featured-post.php), you're trusting that ->post->guid is a reliable indicator of the item's URI. But that's an unfiltered value pulled right out of the database, and thus bypassing all filters (and, by extension, all plugins). In contrast, if you use the function get_permalink instead, you'll automatically inherit the home_url filter, since get_permalink() uses home_url() when concatenating a permalink. Eg:

<h4><a href="<?php echo get_permalink( $cfcw_view->post->ID ) ?>"><?php esc_html_e( $cfcw_view->post->post_title ) ?></a></h4>

This change (and other necessary related changes, such as in the -blog view) should make DM work automatically.

#3 Updated by Dominic Giglio almost 9 years ago

  • Target version changed from Not tracked to 1.4.7

I knew there had to be a disconnect somewhere in my code that was causing Featured Content to not pick up on the work that DM was doing. I kept thinking: when DM is active my code should be returning the DM mapped domain automatically. This explains perfectly why.

Your points 1 and 2 make total sense. That's actually why I asked the question. I figured there must a balance that comes from wanting to provide features your users request while still maintaining your plugin the way it was designed. While I was working on a fix (which was completely opposite from your suggestion) I felt like Featured Content was becoming like a DM extension. It just didn't "feel" right.

I'm moving this to 1.4.7 for right now because the fix should be simpler now and I should be able to get it in by Thursday (I'll punt it if something comes up). I'd also like to get a 1.0.1 released out the door because I've never done it and I'd like some practice with git/svn tagging during a plugin repo maintenance release. :-)

#4 Updated by Dominic Giglio almost 9 years ago

  • Target version changed from 1.4.7 to 1.4.8

I'm moving this to 1.4.8 for now because I'm seeing some weird stuff in my local testing that I'm gonna need a little more time to work out before I can release a 1.0.1 Featured Content updated.

#5 Updated by Dominic Giglio almost 9 years ago

  • Target version changed from 1.4.8 to 1.4.9

#6 Updated by Dominic Giglio almost 9 years ago

Boone,

I think I've got the Domain Mapping plugin issues worked out. The post permalinks were just as easy as you described above, but the blog url was much harder. No matter what I tried I just couldn't get it to pick up the filters from Domain Mapping. I ended up falling back on switch_to_blog() in the static helper class to update the 'siteurl' attribute when MS has been enabled.

I've updated the Github issue and pushed the new code to the 1.0.x branch if you wan to take a look:

https://github.com/cuny-academic-commons/cac-featured-content/commit/58523b6d101c7021a0aec15b2ae5c61d1ca7bb10

I'm gonna need a little refresher, after the original reporter on Github has a chance to do a little testing, on how to merge these changes into master/svn and then tagging for a 1.0.1 release to the plugin repo.

#7 Updated by Dominic Giglio almost 9 years ago

  • Target version changed from 1.4.9 to 1.4.10

#8 Updated by Dominic Giglio almost 9 years ago

Boone,

I'm not hearing back from the OP on the Domain Mapping issue for Featured Content, so I'd like to just push the latest changes and release 1.0.1. I don't think any of the code I added is going to break stuff. At best it resolves their issue and at worst it just doesn't. I think I have a pretty good understanding of how to handle this release but I'd like to get your confirmation if I could please.

I've been developing in a 1.0.x branch similar to our commons workflow. I've committed all the changes I'd like to include in 1.0.1, except changing the version tag. I believe what I need to do is this:

  1. bump version numbers in 1.0.x to 1.0.1
  2. checkout master and merge in 1.0.x changes
  3. checkout svn branch and merge in master
  4. git svn dcommit from svn branch
  5. git svn tag -m "tagging 1.0.1" from svn branch
  6. checkout master and git tag -m "tagging 1.0.1"

I'll then of course push all the branches (and new tag) to Github to make sure the repo's all up-to-date. I've already updated both readme files to correctly reflect all the changes added to 1.0.1. Am I missing anything here?

#9 Updated by Boone Gorges almost 9 years ago

Hi Dom. This workflow sounds good to me. (There are various ways you could do it. You're following a model where the master branch is the reference branch for releases. This is a bit different from the way we do releases on the Commons or in WP/BP - dot-dot releases come from the dot-dot branches. But the master branch model is just fine.) The git-svn stuff sounds right.

#10 Updated by Dominic Giglio almost 9 years ago

OK, cool I had a feeling I was close. You're right, my workflow inside the git repo is slightly different. I am thinking of master as the final stop before release. But we don't exactly follow that workflow on the commons.

I was thinking of trying to get this squeezed into 1.4.10 but now I'm thinking maybe we shouldn't rush it. I'll release tonight so we an make sure the changes work in CBox and then Featured Content 1.0.1 can go into Commons 1.4.11?

#11 Updated by Boone Gorges almost 9 years ago

I'll release tonight so we an make sure the changes work in CBox and then Featured Content 1.0.1 can go into Commons 1.4.11?

Sounds good. I don't think this is in demand on the Commons at the moment in any case.

#12 Updated by Dominic Giglio almost 9 years ago

I think you're correct. The only changes are the ability to choose the HTML element to wrap the widget title and better integration with Domain Mapping. The Commons already uses the appropriate title element and domain mapping isn't as much of a big deal to us as it is to the OP on the Github issue that started this. He runs a bunch of sites on MS and the owners of those sites aren't aware of it. So he really needs his links to appropriately display his customers URLs.

I will release tonight after I've verified everything I need to verify for 1.4.10. I'm leaving this issue in 1.4.10 and will mark it as resolved after the release.

#13 Updated by Dominic Giglio almost 9 years ago

Boone,

OK, so my first plugin point release went fairly uneventfully. Except git-svn stinks. I don't like it at all. I've attached a screenshot of what I was presented with after step three of my workflow above. I think you'll get a kick out of it. :-)

Everything seems to have worked the way it's supposed to. I dcomitted and tagged and then verified on the plugin repo that v1.0.1 is the current version available for download, and it is.

But when I bring up the commons admin in my local env it's not asking me to upgrade. Does it usually take a little while for the changes to propagate through WP's servers? Maybe I need to wait 30 min or an hour for the update to get picked up?

#14 Updated by Dominic Giglio almost 9 years ago

Also, I had a slight readme formatting error, but I'll fix it in 1.0.2.

I logged into the live site and it does list the plugin in the "Update Available" section. So I guess I did OK.

I assume we want the plugin upgraded locally and then pushed out in the next release, right?

#15 Updated by Dominic Giglio almost 9 years ago

  • Target version changed from 1.4.10 to 1.4.11

#16 Updated by Boone Gorges almost 9 years ago

Odd that you had all those conflicts. Did you actually touch that many files between 1.0 and 1.0.x?

Yes, let's upgrade it locally.

#17 Updated by Dominic Giglio almost 9 years ago

I thought it was odd too, I assume it has something to do with the way svn rewrites history?

In this case I did touch that many files. The changes that allow a user to choose the html element that wraps the title required small tweaks across the board.

I was finally able to update in my local env, just took a little longer to show up. However, there's a small bug in the Featured Post autocomplete that's preventing proper results being returned from the Featured Blog. I haven't identified it yet, but I will today or tomorrow and will push 1.0.2 once I have.

Please do whatever testing you need to do on CBox to make sure I haven't broken anything else. I've been trying to find more time to test CBox, hopefully I'll be able to tonight. But my prior testing of CBox didn't uncover any errors other than those that I've already chimed in on. I'll probably do one more round of CBox/Featured Content just to make sure there aren't any more weird bugs with 1.0.1.

#18 Updated by Dominic Giglio almost 9 years ago

Boone,

I just released v1.0.2. As I was researching the autocomplete bug that showed up in 1.0.1 I realized there were some subtle bugs that would have shown up had I not caught them. There were some javascript problems due to the new functionality added in 1.0.1 and also a logic error in the checking of posts/blogs during autocomplete that I found after fixing the first autocomplete bug.

This version should be solid and working well for both us and CBOX installs.

As soon as the new version shows up in my local dashboard I will upgrade and retest, and then push to our repo for 1.4.11. I'll update again here as soon as all that's done.

#19 Updated by Dominic Giglio almost 9 years ago

  • Status changed from Assigned to Resolved

I found one more issue involving the way the js in the admin was identifying multiple widgets. Basically, a second widget instance was pulling posts from the first widget's featured blog because it was reusing it's widget number.

That issue has been fixed and a v1.0.3 has been released to the WP Plugin repo.

I've also updated our repo with the latest version for the 1.4.11 release.

Commit: https://github.com/castiron/cac/commit/6332e8a541f6532ebbbcb81518dd3c89a42a5d2a

Also available in: Atom PDF