Add link to Plugin Editor for each found issue to jump user to file and line of code

[mukeshpanchal27]

Closes #251

Checklist:

• I agree to follow this project's Code of Conduct.

[mukeshpanchal27]

This pull request adds the Plugin Editor link for all error/notice situations. However, there are certain cases where we need to exclude it:

• If a plugin contains .phar or .gitignore files, the Plugin Editor does not support these formats, and clicking the link will display a Sorry, that file cannot be edited message.
• If a plugin does not have a readme file, the plugin will display a no_plugin_readme message, but that file is not present in the folder, so it will show the same error as mentioned above.

I would appreciate hearing your thoughts on this matter before proceeding with the exclusion of these files or any other files that may require exclusion.

[felixarntz]

@mukeshpanchal27 This mostly looks good, though there's one critical problem as this is missing current_user_can( 'edit_plugins' ) checks (also see #262 (comment)).

We must only show these links if the current user has the permission to edit plugins via the built-in file editor - which is something that is often not the case as many environments disable it.

[swissspidy]

We must only show these links if the current user has the permission to edit plugins via the built-in file editor - which is something that is often not the case as many environments disable it.

Should we still show the link if the link has been filtered to point to another editor like VS Code?

[felixarntz]

@swissspidy

We must only show these links if the current user has the permission to edit plugins via the built-in file editor - which is something that is often not the case as many environments disable it.

Should we still show the link if the link has been filtered to point to another editor like VS Code?

Good point, that would make sense.

[mukeshpanchal27]

@felixarntz @swissspidy, in the commit at 733479b, I have addressed all your concerns. Additionally, I have also added checks for the DISALLOW_FILE_EDIT constant, which many plugins use to disable the plugin editors. I have adjusted the result table accordingly.

Could you please review these changes once more? Thank you!

[felixarntz]

Thanks @mukeshpanchal27 for making the updates, sharing a bit of follow up feedback below.

We may need to holistically determine for each table whether or not to include a column for the edit link, by checking for whether any of the rows include a non-empty link.

[felixarntz]

@mukeshpanchal27 Two points of follow up feedback regarding the markup.

[mukeshpanchal27]

@felixarntz @swissspidy @westonruter @joemcgill, I have addressed all the feedback, and the PR is now ready for review.

[swissspidy]

One question, otherwise looks good

[westonruter]

The phpdoc for $filename and $line don't indicate that they can be nullable. Therefore, is this condition valid? If it is, then should they be modified to be nullable in the phpdoc?

[felixarntz]

Fair point, I think we should remove this. If we include checks like this just because someone could pass null (due to lack of parameter type hints), we would need to do that in almost every function.

I do however have another related suggestion: Several of our checks are not line-specific, as in the message data doesn't include a line (or rather have the line set to 0). I think it would make sense to give $line a default value of 0 to clarify that it is optional, and then the method logic could be adjusted to not pass line if it doesn't have an actual (i.e. not 0) value.

[felixarntz]

Fair point, I think we should remove this. If we include checks like this just because someone could pass null (due to lack of parameter type hints), we would need to do that in almost every function.

I do however have another related suggestion: Several of our checks are not line-specific, as in the message data doesn't include a line (or rather have the line set to 0). I think it would make sense to give $line a default value of 0 to clarify that it is optional, and then the method logic could be adjusted to not pass line if it doesn't have an actual (i.e. not 0) value.

[felixarntz]

Thank you @mukeshpanchal27 for the updates, LGTM!

One other comment below, but I think we should solve that in a separate issue/PR.

[mukeshpanchal27]

@westonruter, whenever you have some moment, could you please review this PR so that we can proceed with merging it? Thanks!