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

Fix all broken links/permanent redirects/removed anchors #5553

Merged
merged 6 commits into from
Aug 29, 2015

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Jul 22, 2015

Today, I discovered that Sphinx has a built-in link checker. I couldn't resist running it and fixing everything it found.

It turns out that we had a lot of broken/permanently redirected links.

Q A
Doc fix? yes
New docs? no
Applies to 2.3+
Fixed tickets -

@@ -429,9 +429,7 @@ A great way to see the core functionality in action is to look in the
Redirecting
~~~~~~~~~~~

If you want to redirect the user to another page, use the
:method:`Symfony\\Bundle\\FrameworkBundle\\Controller\\Controller::redirect`
Copy link
Member Author

Choose a reason for hiding this comment

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

Controller API method links are removed as these methods are protected. Only public methods are included in the online API doc.

Copy link
Member

Choose a reason for hiding this comment

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

this should be changed IMO. Protected methods are extension point, so they should be documented in the API doc too (private methods should indeed not appear)

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @stof and IIRC we have/had an issue for @fabpot about this.

@stof
Copy link
Member

stof commented Jul 22, 2015

Is it possible to make this link checker run on Travis ?

and calls the controller. The ``forward()`` method returns the ``Response``
object that's returned from *that* controller::
with the ``forward()`` method. Instead of redirecting the user's browser, it
makes an internal sub-request, and calls the controller. The ``forward()``
Copy link
Member

Choose a reason for hiding this comment

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

Should we remove the comma before the and?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, but let's do that in the docbot PRs and keep this one focussed on URLs.

@javiereguiluz
Copy link
Member

👍 thanks a lot for fixing all these errors!

@wouterj
Copy link
Member Author

wouterj commented Jul 22, 2015

@stof it is possible (it's quite easy, as it's a built-in sphinx builder). However, I don't think it's worth it, as there are many URLs in the Symfony documentation (so running this builder takes some minutes) and not a lot of PRs add new external references. I want to provide a PR status to the contributor as quick as possible, so I don't think making the travis build longer is really needed here.

I thought about only including it in push Travis builds (and not PRs updates). However, we don't get emails about failing builds (and least, I don't get them) and I don't think we'll regularly check Travis to see the status of the branch.

@stof
Copy link
Member

stof commented Jul 23, 2015

@wouterj for push builds, the one receiving the email is the dev doing the push, which means the one doing the merge of the PR.

@wouterj
Copy link
Member Author

wouterj commented Jul 28, 2015

Reverted the removal of all API method links to protected methods, I agree that it should be fixed in the API generator instead.

Ready to merge imo

(fyi, @stof, we decided against adding this to the build during the doc meeting).

@@ -89,4 +89,4 @@ repository and apply changes to the translated documents as soon as possible.
repositories as obsolete documentation is dangerous.

.. _`an ongoing discussion`: https://github.com/symfony/symfony-docs/issues/4078
.. _Symfony docs mailing-list: http://groups.google.com/group/symfony-docs
.. _Symfony docs mailing-list: https://groups.google.com/forum/#!forum/symfony-docs
Copy link
Member

Choose a reason for hiding this comment

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

This one should probably be removed as it has been disabled for quite some time now.

@wouterj
Copy link
Member Author

wouterj commented Aug 22, 2015

I've removed the mailing list link in 185dfbf, as noticed by @fabpot. /cc @weaverryan @xabbuh can you please review & vote?

@weaverryan
Copy link
Member

I just scanned through - looks like all good stuff to me! 👍

@wouterj wouterj merged commit 185dfbf into symfony:2.3 Aug 29, 2015
wouterj added a commit that referenced this pull request Aug 29, 2015
…(WouterJ)

This PR was merged into the 2.3 branch.

Discussion
----------

Fix all broken links/permanent redirects/removed anchors

Today, I discovered that Sphinx has a built-in link checker. I couldn't resist running it and fixing everything it found.

It turns out that we had a lot of broken/permanently redirected links.

| Q | A
| --- | ---
| Doc fix? | yes
| New docs? | no
| Applies to | 2.3+
| Fixed tickets | -

Commits
-------

185dfbf Remove sf-doc mailing list link
80a560f Inline links to Symfony docs
a858888 Fix all invalid API doc links
123d05f Fix all broken links
3c830bc Fix all permanent redirects
09b3b1b Create your own Framework tutorial is moved to the docs
@wouterj wouterj deleted the fix_links branch September 2, 2015 17:43
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.

6 participants