Support from Acquia helps fund testing for Drupal Acquia logo

Comments

babruix’s picture

Assigned: Unassigned » babruix
Issue tags: +prague 2013
babruix’s picture

Assigned: babruix » Unassigned
Status: Active » Needs review
Issue tags: -prague 2013
FileSize
3.49 KB

Initial patch changes tokens and uses \Drupal::url
Not sure about changes in module and path system, should some information be changed?

Status: Needs review » Needs work

The last submitted patch, path-module-hook-help-2091353-2.patch, failed testing.

babruix’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Someone needs to go through the help that is here and see if what it says still matches the Drupal 8 user interface. If you would like to do that before you make a new patch, that would be helpful.

Other changes needed:

a) All drupal.org links should be changed to https.

b) Links are still not using the correct formatting using routes instead of paths. See
https://drupal.org/node/632280#url-note

Thanks! And sorry for not reviewing this sooner -- I'm still behind from the great Prague help sprint.

batigolix’s picture

Component: documentation » path.module
Status: Needs work » Needs review
FileSize
3.48 KB

Patch fixes a) & b) from #5

The text matches to current UI which seems not have undergone major changes for D8

Setting component to path.module to let maintainers chime in

I'm wondering if the array('fragment' => 'module-path') piece in the permissions link is actually supposed to work in combination with overlay. The resulting url is http://d8.local/#overlay=admin/people/permissions%23module-path
I'm trying this with overlay but the browser does not navigate to this anchor ...

jhodgdon’s picture

Issue tags: +Needs manual testing

Yeah, fragments do not work with the overlay. However, they can be tested by turning off the overlay, and they're helpful for people who don't use the overlay.

Anyway, the patch in #6 looks fine. Can someone give it a manual test to see that:
a) All the links go to where they should be going
b) The names of pages/sections mentioned in the help text match the actual names of the pages in the administrative UI... I guess the only one mentioned is on content editing it says "URL path settings" -- can it be verified that this is still what it says?

Thanks!

jhodgdon’s picture

Status: Needs review » Needs work

The last submitted patch, 6: path-module-hook-help-2091353-4.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review
batigolix’s picture

Issue summary: View changes
Issue tags: +Novice

Need test. See #6

jhodgdon’s picture

Status: Needs review » Needs work

I gave this a manual test today and it all looks great except for one conceptual problem.

The "Creating" uses item says that you can create aliases when you create content. This is true for Nodes, but one of the examples it gives is for a user alias, and I am not seeing the ability to create an alias for a user page when I create the user page. With only the core User and Path modules, I think if you wanted to create an alias for user/3 you would need to do it from the alias list, not while you are creating the user.

And the field where you can add a URL alias on a Taxonomy term is not called the same thing. It just says "URL alias" instead of "URL path settings".

So I think this is a bit misleading. I'm not sure what the best way to fix it would be, and I'm not sure why the URL alias is not available for users and looks different for taxonomy terms either. Any comments from the Path maintainers?

batigolix’s picture

This patch attempts to fix the problem raised in #12. The style is not very refined but I think it is clear and not misleading anymore. Suggestions for improvement are always welcome.

I don't think the labeling is different. In the case of the Node it is the "vertical tab label" (form element 'type'= '#details' ?) that is named "URL path settings". The actual input field for both taxo term and node are labeled URL alias.

jhodgdon’s picture

Status: Needs review » Needs work

Better -- I think that is much clearer.

I missed this in the last review, whoops: The link to the online docs needs updating to our standard wording (still says "handbook").

I'm also wondering, since all the field/section names on the taxonomy/node edit pages are in "Sentence case", if the text would be clearer if they were in quotes? Such as

... under the section URL path settings in the field URL alias ...

vs.

... under the section "URL path settings" in the field "URL alias"...

?

batigolix’s picture

Status: Needs work » Needs review
FileSize
3.53 KB
3.81 KB

Ok. fixed remarks from #14

jhodgdon’s picture

Looks good to me, thanks!

Needs a manual test:
- Verify that all the links work
- Verify that all mentions of pages/text within the UI match what is seen in the UI
- Verify that the formatting is OK.

Juterpillar’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
27.69 KB

Have tested this on simplytest.me and verified that it passes html validation and spelling/punctuation is error free.

Here's the screenshot with the patch applied.

jhodgdon’s picture

Component: path.module » documentation

Thanks for testing! Moving back to Docs component so I see it next time I'm doing commits.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review

A major change to the Path module was just committed:
#1980822: Support any entity with path.module
so I think we should review this help again and make sure it still matches the UI. I'll also click "retest" just in case.

jhodgdon’s picture

jhodgdon’s picture

The UI for this module may not have changed yet. See #2201051: Convert path.module form alters to a field widget

jhodgdon’s picture

Status: Needs review » Postponed
jhodgdon’s picture

Status: Postponed » Needs work

We just had a change to hook_help, on this issue: #2183113: Update hook_help signature to use route_name instead of path.

Here is the change record: https://drupal.org/node/2250345

This patch will need a reroll for this change.

Also, un-postponing since the issue it was postponed on has been taken care of.

gnuget’s picture

Issue tags: +Needs reroll
mrjmd’s picture

Assigned: Unassigned » mrjmd

Currently working on this reroll

mrjmd’s picture

Status: Needs work » Needs review
FileSize
3.83 KB

Rerolled.

mrjmd’s picture

Assigned: mrjmd » Unassigned
mrjmd’s picture

Issue tags: -Needs reroll
jhodgdon’s picture

Thank you for the reroll!

So since the UI for the Path module changed (see comments above), we need someone to test this help manually again:
- Verify that all the links work
=====> Especially this: Verify that all mentions of pages/text within the UI match what is seen in the UI
- Verify that the formatting is OK.

gnuget’s picture

I just did a my manual review.

This is the list of links with a screenshot of what is displayed when you click on them or where point them if the links display an external site:

- Pathauto: https://drupal.org/project/pathauto

- online documentation for the Path module: https://drupal.org/documentation/modules/path

- URL aliases:

- Create and edit URL aliases:

- list of all aliases:

I have a question, the links which are pointing to an external site (Pathauto, online documentation for the Path module) shouldn't them open in a new tab/window?

jhodgdon’s picture

We are not currently opening links from help in other windows.

@gnuget: I could not tell from your review in #30: did you find any problems, or did you think everything was fine? If everything is fine, please set the issue status to "Reviewed and tested by the community". If there is a problem, set it to "needs work" and explain what the problem is. Thanks!

gnuget’s picture

Status: Needs review » Reviewed & tested by the community

Hi.

I'm not sure why the images aren't displayed in my comment :-|

I didn't find any problems, i will change the status of this issue.

Thank you.

jhodgdon’s picture

Thanks! This is ready to go in then.

RE images - spaces do not work in image names for whatever reason. Bug in the contrib module that does this.

Dries’s picture

What is the reason for changing "@aliases" into "!aliases"?

jhodgdon’s picture

All of the hook_help() implementations are being updated in this way for URLs. We discussed it a while back on the parent issue. We don't need URLs that are already generated with url() or a similar function/method to be further sanitized by check_plain(), so we're using !name rather than @name to put them in t() and similar methods.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: path-module-hook-help-2091353-26.patch, failed testing.

amitgoyal’s picture

Status: Needs work » Needs review
FileSize
1.94 KB

@jhodgdon - The patch in #26 no longer applies in current core because all the suggested changes are already in core. Somebody has already committed these changes in core.

Although there is one small issue in the core Path module. As per the Help text standard at https://drupal.org/node/632280, the wording and link should be like "For more information, see the online documentation for the
Path module
."

Whereas currently in core, it is like "For more information,
see the online documentation for the Path module
."

Please see the attached patch which simply makes the link on "Path module" wording rather "see the online documentation for the Path module".

jhodgdon’s picture

Status: Needs review » Fixed

No, this change is incorrect. The edit you made to the help standards page is incorrect and the current Path module text is correct. I will update the standards page.

amitgoyal’s picture

Thanks @jhodgdon for the feedback and correcting it!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.