NPRML Construction Changes

[jwcounts]

NPRML class uses wp_get_attachment_url() instead of attachment GUID
Deduplication between direct attachments and enclosures

[tamw-wnet]

Is this a bugfix, or an enhancement?

I do not use the "push" feature of the plugin so I dont' feel qualified to review. I think we should make a point of reviewing all PRs though. @davidmpurdy might be qualified to do so?

[jwcounts]

A bugfix for our site, but an enhancement in general. Typically, when pushing an article from the site to the NPR API, it would list attachment URLs as the attachments GUID. On a vanilla Wordpress install that would be fine, but we use WP Offload Media to store all of our attached files in Amazon S3, which means that the GUID doesn't work.

But substituting wp_get_attachment_url() for the GUID, it allows for the NPR API to support sites that don't store attachments locally.

Side note: we've been using this modification in production for at least a year without errors or issue.

[tamw-wnet]

A bugfix for our site, but an enhancement in general. Typically, when pushing an article from the site to the NPR API, it would list attachment URLs as the attachments GUID. On a vanilla Wordpress install that would be fine, but we use WP Offload Media to store all of our attached files in Amazon S3, which means that the GUID doesn't work.

But substituting wp_get_attachment_url() for the GUID, it allows for the NPR API to support sites that don't store attachments locally.

Side note: we've been using this modification in production for at least a year without errors or issue.

This makes sense, using the GUID as the attachment URL is bad practice anyway, that's not even reliably the real URL within a vanilla WP install.

[davidmpurdy]

We've been using something similar in production for years (I probably got it from you originally).

One question: Should the $image_guid variable have a different name since it's really the URL? (Sorry if this is pedantic - I don't review a lot of others' code! Just thinking of all the existing confusion in the world of what GUIDs are).

[davidmpurdy]

And I would call it a bugfix, full stop. Even if the bug doesn't surface on sites where the GUID happens to be the URL, it still seems to me like it's a bug that doesn't cause problems on some sites.

[jwcounts]

We've been using something similar in production for years (I probably got it from you originally).

One question: Should the $image_guid variable have a different name since it's really the URL? (Sorry if this is pedantic - I don't review a lot of others' code! Just thinking of all the existing confusion in the world of what GUIDs are).

Seems extraneous, but for continuity, we might as well. Just need to make sure that we update it everywhere and make sure not to clobber any other variables.

[tamw-wnet]

I'll leave to you guys to decide when this is ready to merge -- I'm guessing pending @jwcounts tweak to the variable name.

[jwcounts]

Variable name has been changed from $image_guid to $image_attach_url

I realized on the previous go-round that I named the variable $image_guid because the previous call was for $image->guid, and it was easier to change the -> to a _

[tamw-wnet]

Looks good to me

[tamw-wnet]

Either of you guys should've felt free to merge this pull request btw, we're a cheerocracy.