Skip to content

add missing crosslinks between authorization and voters #3120

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

Closed
wants to merge 2 commits into from

Conversation

monbro
Copy link

@monbro monbro commented Oct 28, 2013

I experienced a lack of internal crosslinks. Especially when I was faced with the page: http://symfony.com/doc/current/components/security/authorization.html#access-decision-manager

I was not able to find the configuration snippet as fast as it could be. I came from a search on google to the page and as a user I should be able quickly to jump to any place with related content from this point.


On the page http://symfony.com/doc/current/cookbook/security/voters.html#changing-the-access-decision-strategy the description about the different strategies is not good as on the other one as well. Thats another reason why I added a backlink.

PS: Sorry, Iam not sure if the links correct in this way I added them, I tried. I can also change the box 'note' to a 'tip', if that makes more sense.

@wouterj
Copy link
Member

wouterj commented Oct 29, 2013

Thanks for starting this! Really appreciated.

However, make sure you break the lines after the first word that crosses the 72th character and maybe change .. note:: to .. seealso::?

@monbro
Copy link
Author

monbro commented Oct 29, 2013

@wouterj alright, I changed them to seealso:: and I checked each seealso:: and each has a prepended blank line.


.. seealso::

You can change the default strategy in the configuration: :ref:`book-security-voters-changing-the-access-decision-strategy`
Copy link
Member

Choose a reason for hiding this comment

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

I would use configuration as a label for the link:

.. seealso::
    You can change the default strategy in the :ref:`configuration <book-security-voters-changing-the-access-decision-strategy>`.

@monbro
Copy link
Author

monbro commented Oct 30, 2013

I updated the pull request according to @xabbuh suggestions.


.. seealso::

You can change the default strategy in the :ref:`configuration <book-security-voters-changing-the-access-decision-strategy>`.
Copy link
Member

Choose a reason for hiding this comment

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

this needs a line break before :ref:

Also, the referencenames (book-security-voters-changng-the-access-dicision-strategy and components-security-authorization-access-decision-manager) do not exists. I would also prefer to use shorter names, e.g. security-voters-change-strategy and components-security-access-decision-manager. For more info about references: https://gist.github.com/WouterJ/5276618

@monbro
Copy link
Author

monbro commented Oct 30, 2013

I updated both links with a prepending line break and changed the links to @wouterj suggestion.
PS: Sorry, I am not that familiar with the doc syntax in here, is there a proper doc on that somewhere?

@wouterj
Copy link
Member

wouterj commented Oct 30, 2013

You are still missing the pointers. Sphinx doesn't know anything about the references now.

And no, there are no really nice docs about our syntax. We are using Sphinx, an extension of reStructured Text. Both contain only hard to read spefications and references. The only good way to learn is to learn it 'on the fly'

@monbro
Copy link
Author

monbro commented Oct 30, 2013

Do I got it now? :-)

@wouterj
Copy link
Member

wouterj commented Oct 30, 2013

Yes, 👍 . If you know how to do it, could you please squash the commits?

@monbro
Copy link
Author

monbro commented Oct 30, 2013

Alright I squashed them into one!

@xabbuh
Copy link
Member

xabbuh commented Oct 31, 2013

👍 Your local merge is also part of this PR. But, I think @weaverryan can cherry pick de233b9 and apply it to the 2.2.

@monbro
Copy link
Author

monbro commented Oct 31, 2013

@xabbuh yeah I tried to include that one, but it's ging to be split up in a bunch of commits, or maybe I am still a retard ;-)

@weaverryan
Copy link
Member

Thanks everyone - this was an easy merge after all the conversation! @monbro, I've patched your commit into the 2.2 branch at sha: 292d7c0

Cheers!

@weaverryan weaverryan closed this Nov 3, 2013
xabbuh added a commit to xabbuh/symfony-docs that referenced this pull request Nov 4, 2013
weaverryan added a commit that referenced this pull request Nov 5, 2013
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