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

Avoid links to expired articles #508

Merged
merged 4 commits into from Aug 15, 2015
Merged

Avoid links to expired articles #508

merged 4 commits into from Aug 15, 2015

Conversation

makss
Copy link
Contributor

@makss makss commented Jul 28, 2015

Discussion: http://forum.textpattern.com/viewtopic.php?id=46062

  • For expired articles is returned link to site_url/#expired_article
  • Show warning permlink_to_expired_article if production_status != 'live' && txpinterface == 'public'

@Bloke
Copy link
Member

Bloke commented Jul 28, 2015

Great stuff, thanks. I like the optimisations in makss@96ded57. Much cleaner.

Couple of things to note about makss@9c901bf:

  • This function is also used on the admin side where we probably don't want it to trigger. For example, set up a few articles with expiry times in the past, then on the Articles panel, do any bulk operation on a bunch of the articles, including the expired ones: you'll get the error triggered if your production status is not Live. Are there any other instances when it's undesirable to trigger it? I haven't checked all the places where it's called.

  • When triggering a notice, it's better to include supplemental info as params to gTxt() so they get passed to the string for replacement. e.g.:

    trigger_error(gTxt('permlink_to_expired_article', array('{id}' => $thisid)), E_USER_NOTICE);

But on the whole I think this patch is a step in the right direction, so if you could fix these issues and check the warning isn't thrown in places where it's not wanted, I'm happy to merge it in.

Oh, and btw, PSR-2 prefers spaces around operators such as < :-)

@makss
Copy link
Contributor Author

makss commented Jul 29, 2015

You are right, a warning is necessary only on Public side. Thank you for the hint of gTxt().

Bloke added a commit that referenced this pull request Aug 15, 2015
Avoid links to expired articles. Thanks, makss
@Bloke Bloke merged commit b4feddc into textpattern:master Aug 15, 2015
@makss makss deleted the patch-3 branch August 15, 2015 15:48
@rwetzlmayr
Copy link
Contributor

For expired articles is returned link to site_url/#expired_article

I think we already had the proper solution regarding our reply to requests for expired articles. We sent a '410 Gone' response code in conformance with RFC7231.

Switching from a real URL to the fake fragment marker #expired_article in the URL will disturb monitoring and analytics tools and generally make Textpattern a worse web citizen.

@rwetzlmayr rwetzlmayr self-assigned this Aug 20, 2015
@makss
Copy link
Contributor Author

makss commented Aug 20, 2015

Referring to expired articles - this is a mistake web developer. This patch provides a warning about such links and makes the lesser of all evils - refers to the root of the site with a marker /#expired_article. This marker will be visible in web analytics and webmaster will draw attention to a problem with the site.

It would be possible to add id article in marker, but I think it's unnecessary.

@rwetzlmayr
Copy link
Contributor

This marker will be visible in web analytics

This is an assumption that only applies to analytics with client-side sensors (e.g. Google Analytics).

The fragment identifier is not sent to the server. It is therefore not visible in the server's access logs, the Textpattern log panel, and the like.

@makss
Copy link
Contributor Author

makss commented Aug 21, 2015

I will not object to the removal of the marker #expired_article, important to remain alert trigger_error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants