Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
I18n: Add check to detect string wrapped in HTML #1856
I18n: Add check to detect string wrapped in HTML #1856
Changes from all commits
a43ae6d
63ae721
20b87f2
51f9f6e
1294413
cf3fcb3
70f9032
c56f18a
005c198
6663aef
45abbf6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still like to see some additional tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added your example and another variation of an existing one, but seem to struggle to come up with meaningful ones overall. Could you give me some pointers as to what you're thinking of?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, try this as a good starting point for finding more tests: try running the sniff over WP core and see which false positives it throws up for this new error code. Those cases will all need to be accounted for and the patterns found which caused the false positives should be covered by unit tests.
Also see the link in the original issue to Core trac where there are also quite some examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, makes sense, thanks.
wp-includes
doesn't yield any occurrences,wp-content
has the following:https://github.com/WordPress/WordPress/blob/c289bb59acdd3217cdb81e15b580dc3d7cd6a007/wp-content/themes/twentythirteen/image.php#L30
https://github.com/WordPress/WordPress/blob/c289bb59acdd3217cdb81e15b580dc3d7cd6a007/wp-content/themes/twentyeleven/inc/theme-options.php#L121-L122
Those seem legit, but I think we've covered that class of issue with our current unit tests, no?
wp-admin
has quite a few, e.g.https://github.com/WordPress/WordPress/blob/c289bb59acdd3217cdb81e15b580dc3d7cd6a007/wp-admin/comment.php#L56-L57
https://github.com/WordPress/WordPress/blob/c289bb59acdd3217cdb81e15b580dc3d7cd6a007/wp-admin/edit-comments.php#L194-L197
https://github.com/WordPress/WordPress/blob/c289bb59acdd3217cdb81e15b580dc3d7cd6a007/wp-admin/edit-form-advanced.php#L313-L314
and so on.
A lot of these are
<a />
links where only the link text needs translating, and the actual<a />
could be moved out of the__()
call.Uh, not sure which issue you mean 😅 Maybe this list (which #1419 (comment) refers to)? I checked a few of those.
These also seem pretty much covered I'd say 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but that's not true. Those are legit translation calls including the tag as there may be a localized page available for that URL and part of translating is changing the URL to the localized version.
I think it would be a good idea to allow for those, i.e. if the tag is an
a
AND has ahref
with a hard-coded URL, there should be no warning.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I didn't think of that.
I'm not totally sure it should be generalized that much though? This seems like a fairly 'semantic' addition to the sniff that implies quite some background information (there can be localized link targets), which might not be true for all codebases alike. One could argue that the localized link targets are more the exception than the rule, that the sniff doesn't necessarily need to know about them, and that it'd be fine to require a sniff ignore to be added by the author in case it's needed 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the warning would be kept for link tags with hard-coded URLs, I'd like to see a separate error code for those.
That would at least allow codebases - including WP Core - which do have localized URLs and therefore do this with good reason to exclude the warning for those particular text strings from their custom ruleset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added per 1294413