coyote-team / coyote-wp

Wordpress Plug-in for Coyote
2 stars 2 forks source link

Duplicate Resources after image upload #75

Closed NolanHill closed 3 years ago

NolanHill commented 3 years ago
  1. Uploaded image to Media Library in WordPress
  2. "Manage image on Coyote website" Links to Coyote resource
  3. Looking through recently added resources in Coyote reveals that there are two versions of the resource. One version is for http and the other is for https
  4. The media selector "Manage image on Coyote website" link takes you to the http version, but the alt text on the page is drawn from the https version
sinabahram commented 3 years ago

@jkva can we use "//" instead of specific protocols? But, also looping in @flipsasser on this issue as Coyote should be potentially modified to do the same.

flipsasser commented 3 years ago

I'll need to determine how many dupes this could potentially create in production before I make the change, but once I know I'll start stripping protocol off of resources as they're added

jkva commented 3 years ago

Via https://github.com/coyote-team/coyote-wp/commit/c772d5e82a78529ca1f9620be12f425f1266b6ed any attachment url is now explicitly set to HTTPS. While I could strip off the protocol, it makes no sense for a WP installation to be running on a non-HTTPs connection.

sinabahram commented 3 years ago

Understood, however isn't that just trading the problem, @jkva. It is why I was asking about // instead of http or https.

jkva commented 3 years ago

There was more than one place in the code where attachment urls were loaded, this is now only done via a helper function which always maps to a https:// url, which should prevent duplicate protocol image attachment urls from being used to create resources.

sinabahram commented 3 years ago

Right, but what if the site is http only?

jkva commented 3 years ago

Then the attachments won't resolve. The question is whether we should be encouraging WordPress installations that installed this plugin from not running SSL. Given most browsers already throw up a big warning I don't see the point. But if you're hard set on it I can strip the protocol.

sinabahram commented 3 years ago

I hear you on SSL, but it is not that simple. Staging and other dev environments may not have SSL for folks. Even some of the top hosting sites don't seamlessly offer that on their dev pipelines, which is a problem.

So, that's why I'm asking about a protocol agnostic way. Why is it not possible to check for this and act appropriately?

jkva commented 3 years ago

Fair point. Testing environments were my one case where I could consider a non-SSL connection. I'll make it protocol-agnostic.

flipsasser commented 3 years ago

I have a change for that on my end as well, Job. It looks for resources while ignoring the protocol.

On Thu, Dec 3, 2020 at 9:20 AM Job van Achterberg notifications@github.com wrote:

Fair point. Testing environments were my one case where I could consider a non-SSL connection. I'll make it protocol-agnostic.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/coyote-team/coyote-wp/issues/75#issuecomment-738154973, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACDEQUWYYCCCN2IZSX56DSS7CGJANCNFSM4UAANQCQ .

jkva commented 3 years ago

I've been testing with this, for instance over at https://staging.coyote.pics/organizations/43/resources/316674

Seems to work fine, the only thing I notice is that now the image is initially captioned (if without a caption) as //coyote.pac.clients.3ode.nl/wp-content/uploads/2020/12/foo-bar-baz.jpeg.

Other than that, no issue.

jkva commented 3 years ago

It would also require any installations to import again, since this changes the source_uri for any attachment image already processed.

sinabahram commented 3 years ago

hi @jkva I don't understand your last two comments. Would you please explain further, illustrating with a few examples?

jkva commented 3 years ago

The plugins tracks known resources via a hash calculated of the image's source. Since this source now has changed when dealing with attachments, any pre-existing hashes for those attachments will no longer be valid.

sinabahram commented 3 years ago

That's only true of existing installations to date, though, right? You're not talking about something that will happen on fresh installations in the future? We don't have any actual installs of this plugin (which means that problem will never happen), so that's why I'm wondering about this comment and want to make sure I fully understand your point.

jkva commented 3 years ago

Yes, that's correct. It's a technicality.

jkva commented 3 years ago

As for the earlier comment, all that means is that before, if the resource wasn't created with an explicit title, it falls back to the source uri. Those now look a bit strange without the protocol in them, but again, probably not a big deal. I just didn't want to leave it out.

sinabahram commented 3 years ago

I understand the above to be as follows. Please confirm.

1: If http changes to https, this will have zero effects what-so-ever on the plugin or the ability to look up images, etc.

2: The reverse of 1 is true with https turning into http

3: @flipsasser propper protocol handlers will be added back in Coyote side, because otherwise we've just broken all the links.

4: @jkva removing this protocol handler in no way adversely affects anything on fresh installs.

jkva commented 3 years ago

I can confirm 1, 2, and 4.

jkva commented 3 years ago

Closing as addressed plugin-side.

flipsasser commented 3 years ago

Cool - @Sina Bahram sina@sinabahram.com the answer is yes, we only perform lookups for an upsert without protocol but the source URI remains whatever we were initially given

On Wed, Dec 9, 2020 at 11:09 AM Job van Achterberg notifications@github.com wrote:

Closed #75 https://github.com/coyote-team/coyote-wp/issues/75.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/coyote-team/coyote-wp/issues/75#event-4091541457, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACDETVBUAHX3I24PVYFS3ST7DPXANCNFSM4UAANQCQ .

sinabahram commented 3 years ago

Amazing @flipsasser thank you.