API page: https://api.drupal.org/api/drupal/core%21modules%21help%21help.api.php/f...

Enter a descriptive title (above) relating to function hook_help, then describe the problem you have found:

As per the "Deprecated" notice on public static function Drupal::url

Drupal::url(...) is deprecated and we should us Url::fromRoute(...)->toString() instead. Also the array doesn't follow the coding standard https://www.drupal.org/coding-standards#array

this means ...

array(':blocks' => \Drupal::url('block.admin_display'))

should look like this

[':blocks' => Url::fromRoute('block.admin_display)->toString()]

CommentFileSizeAuthor
#8 2730625-8.patch1.53 KBgaurav.pahuja
#5 hook_help_api_example_deprecated-2730625-5.patch1.52 KBpashupathi nath gajawada

Comments

dbt102 created an issue. See original summary.

cilefen’s picture

cilefen commented
Title: HELP "Code" is deprecated » hook_help API example contains a deprecated call to Drupal::url()
cilefen’s picture

cilefen commented
Issue tags: +Novice
pashupathi nath gajawada’s picture

pashupathi nath gajawada commented
Assigned: Unassigned » pashupathi nath gajawada

I am working on it.

pashupathi nath gajawada’s picture

pashupathi nath gajawada commented
Status: Active » Needs review
StatusFileSize
new hook_help_api_example_deprecated-2730625-5.patch1.52 KB

Hi @dbt102 ,

As suggested replaced the deprecated method .
Please find the patch attached.

cilefen’s picture

cilefen commented
Status: Needs review » Needs work

The issue is about the API example of hook_help. You patched block_help.

The last submitted patch, 5: hook_help_api_example_deprecated-2730625-5.patch, failed testing.

gaurav.pahuja’s picture

gaurav.pahuja commented
StatusFileSize
new 2730625-8.patch1.53 KB

Giving it a try.

gaurav.pahuja’s picture

gaurav.pahuja commented
Status: Needs work » Needs review
dbt102’s picture

dbt102 commented

thanks @guarav, @cilefen and @Pashupathi for responding to this issue.

This is my first time submitting an issue on https://api.drupal.org/api/drupal/core ... and I am not understanding what I am seeing.

What I mean is that, I was expecting that an authenticated user who had access to the api documentation would review this comment and just go in and make the edit.

@cilefen first reviewed the issue and tagged it as +Novice.

Then @Pashupathi and @guarav both created patches for review/comment.

What is it that you are patching against and how would I get a copy of that so I could actually help with the review?

It looks like the patches are against core itself? What I commented on was just the example mentioned in the API example. So, are these API examples drawn on directly from core? In other words, if these patches are approved and committed, will it actually change both the code in the core module as well as the api example code at the same time? or is the patch committed against core, then someone manually goes in and updates the api documentation?

I assume there is Drupal documentaion somewhere on this, but I have not found it yet.

cilefen’s picture

cilefen commented
Status: Needs review » Needs work

@dbt102 Yes, most of the API documentation is generated from code comments in the software. This is typical.

@gaurav.pahuja Do you see the bug in the change you made?

[':blocks' => Url::fromRoute('block.admin_display')->toString()])

dbt102’s picture

dbt102 commented

thanks @cilefen for the reply

I see now the Create your own API Site in Drupal 7 as well as how the Documentation for core is maintained w/ git

I also found this link Updating API documentation, so I guess this is actually the workflow that is being followed in this issue queue.

that's a big help

dbt102’s picture

dbt102 commented

OK, I think I see now the point of my confussion. Guess I never really paid attention, but api.php is actually in core when downloaded. So api patches are applied directly to core.

So I applied 2730625-8.patch by @gaurav.pahuja and it applied cleanly. I reviewed the help.api.php file and see the changes have been made as I expected.

gaurav.pahuja’s picture

gaurav.pahuja commented

@cilefen,

Can you please let me know the issue with the code?

Thanks,
Gaurav

cilefen’s picture

cilefen commented

This is not a valid PHP expression. There is an extra parenthesis.

[':blocks' => Url::fromRoute('block.admin_display')->toString()])

dbt102’s picture

dbt102 commented

The statement in #15 closes a t() fucntion, so the last parenthesis is used for that

gaurav.pahuja’s picture

gaurav.pahuja commented
Status: Needs work » Needs review

Thanks @dbt102.

Marking for review again.

cilefen’s picture

cilefen commented

Oh, my bad.

snehi’s picture

snehi commented

Looks good to me.

sharique’s picture

sharique commented
Status: Needs review » Reviewed & tested by the community

Patch applied cleanly, drupalcs give any serious error.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue rescope

This patch looks OK, but don't we have another issue that fixes all calls to Drupal::url() elsewhere? It seems silly just to fix this in this one place in this patch, when there are calls to this function all over core. Our Issue Scope policy suggests that we should combine this type of change into a bigger patch.
https://www.drupal.org/core/scope#big-picture

dbt102’s picture

dbt102 commented

@jhodgdon ... the first I learned of the issue was via @tduong, commenting on patch by @ifrik #2702561-22: Provide a hook_help text.

At that point we marked Help text standard (for core and contrib) with revision notes, but I'm not aware that an actual 'big-scope' issue has been created at this point.

I'd be glad to take that on (create the issue) if one has not been started.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Well, the first thing to do would be to search Core issues for Drupal::url() and see if there is another issue where these calls are being removed.

If there isn't one, then this one could be expanded to cover all of Core.

If there is already one, then this one should be closed as a duplicate.

Thanks!

dbt102’s picture

dbt102 commented

Thanks for the guidance @jhodgdon.

So, per #23 I searched Core for Drupal::url() and found none. I kind of think this has already been picked up in CODER, but need to double check that #2731667: Update public static function Drupal::url

In terms of "Getting the big picture" we are only talking about a couple lines of code and 164 calls to Drupal::url() per the api.

However, in terms of scope, there is this other issue #2135291: [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8 with a long history. I only bring it up to be clear about what we are addressing when 'broadening' this issue.

In the Issue summary of this immediate issue, the fix shown includes two parts ...

1. the fix to Drupal::url()

AND

2. the short array syntax.

Closure does not seem to have been reached on the short array syntax, but it might be getting close? ref: #2135291-75: [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8

I guess I should cross reference this comment over there #2135291-76: [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8 as well to see if there is any interest in piggy backing on a combined patch?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

OK, let's see.

So regarding the Coder issue, I went ahead and closed it, because deprecated functions are a special case that I don't think we want to handle case-by-case in Coder. Sorry the instructions were unclear on that, and thanks for diligently reading and following the instructions!

So. What I was talking about was those 164 calls to Drupal::url(). We should not remove this one call to Drupal::url() in this one issue. We should instead have a bigger issue/patch that removes all 164 of them.

Regarding the [] vs. array() syntax... fixing all of Core for that would probably be a huge, impossible task, but we should leave it to that other issue to decide and/or do it.

But I agree that meanwhile, we should fix the hook_help() function body in this way... maybe a good scope for that would be "Fix all hook function bodies so that the example code uses [] instead of array() syntax"? I think that would be doable, and then we'd have good examples in all of our hook bodies.

So... that would mean we should probably close this issue and create two different ones:
a) Replace all 164 calls to the deprecated Drupal::url() function in Core.

b) Fix all hook function bodies so that their example code uses [] instead of array() syntax.

Does that make sense as a plan?

dbt102’s picture

dbt102 commented
Assigned: pashupathi nath gajawada » dbt102

Good plan.

I'm assigning this task to myself for now, and will create the two new issues. Once that is done I come back to this issue and close with refernece to the two new issues.

dbt102’s picture

dbt102 commented

So... that would mean we should probably close this issue and create two different ones:
a) Replace all 164 calls to the deprecated Drupal::url() function in Core.

--> #2731817: Replace all calls to the deprecated Drupal::url() function in Core

b) Fix all hook function bodies so that their example code uses [] instead of array() syntax.

--> #2731835: Fix all hook function bodies so that their example code uses [] instead of array() syntax.

dbt102’s picture

dbt102 commented
Status: Needs work » Needs review

Completed this issue per #25.

Marking this as "needs review" just to make sure the intent of the fix is properly called out in the two new issues.

dbt102’s picture

dbt102 commented
Assigned: dbt102 » Unassigned
dbt102’s picture

dbt102 commented

OK, I've done back and reviewed what I wrote last night, and think it clear enough to move forward.

dbt102’s picture

dbt102 commented
Status: Needs review » Closed (duplicate)

Marking as a closed (duplicate) since the two new big-picture issues now cover this one

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Thanks!!