Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "disable-attachment-pages" module #24

Merged
merged 11 commits into from
Jul 6, 2020
Merged

Add "disable-attachment-pages" module #24

merged 11 commits into from
Jul 6, 2020

Conversation

runofthemill
Copy link
Contributor

Added code from https://gschoppe.com/wordpress/disable-attachment-pages/ - I tried to match the structure to other modules, but other than making the function names camel case the code is unchanged.

There's a filter available (gjs_attachment_slug_prefix) which I'm not sure is worth keeping, and an unused function (makeAttachmentsPrivate()) which I asked about and got this response:

The gjs_attachment_slug_prefix filter is provided so that users can change the default of wp-attachment to another value, in case they need to for some reason. This is important because the slug wp-attachment-«original-file-slug» will still be reserved by WordPress, so it is possible that on some sites this could cause collision issues.

make_attachments_private(){} was originally hooked to the register_post_type_args filter, as it should cause the attachment post type to not reserve slugs. However, attachments currently ignore the standard post type arguments. I’ve re-added this hook to the to the plugin, in the hope that someday, WordPress may add support these arguments properly.

I haven't done thorough testing, but enabling this module w/ intervention does indeed disable attachment pages, rewrites the URLs, and seems to work as advertised!

Copy link

@knowler knowler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be great. I just noticed a small issue which I don’t think was intentional.

composer.json Outdated
@@ -1,5 +1,5 @@
{
"name": "soberwp/intervention",
"name": "runofthemill/intervention",
Copy link

@knowler knowler Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☝️ I’m assuming this wasn’t meant for the PR. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, that was so I could use it with composer - forgot subsequent commits would affect the PR =D

I'll fix now :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@knowler fixed!

@darrenjacoby
Copy link
Owner

@runofthemill are you able to update the README.md?

@runofthemill
Copy link
Contributor Author

I am - but I just noticed an issue when trying to upload media, so I'm going to look into that as well!

@darrenjacoby
Copy link
Owner

@runofthemill thanks!

@runofthemill
Copy link
Contributor Author

@darrenjacoby finally got around to looking at this. fixed the issue, but honestly I'm still a little perplexed by the gjs_attachment_slug_prefix filter - I can't figure out where wp-attachment is used ¯_(ツ)_/¯

Lemme know if you want to keep or drop that filter, then I'll update the readme.

@darrenjacoby
Copy link
Owner

darrenjacoby commented Dec 29, 2018

@runofthemill thanks! I think we should drop the gjs_attachment_slug_prefix filter, what do you think?

@runofthemill
Copy link
Contributor Author

I'll drop it, don't think it's necessary...can always revisit later if necessary!

@runofthemill
Copy link
Contributor Author

@darrenjacoby removed & README updated ✌️

@darrenjacoby
Copy link
Owner

@runofthemill thanks! I'll get this merged soon, I'm going to be doing some other updates to intervention at the same time.

@runofthemill
Copy link
Contributor Author

I mean I only took 6 months to get this finished.... :P

@LukeAbell
Copy link

Any chance we could get this merged? Thanks!

@darrenjacoby
Copy link
Owner

Merging and will release as part of a new version soon.

@runofthemill 6 months pales in comparison to my 1.5 year merge timeline it seems 😬

@darrenjacoby darrenjacoby merged commit 64b6325 into darrenjacoby:master Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants