Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Munavijayalakshmi created an issue. See original summary.

Munavijayalakshmi’s picture

Assigned: Munavijayalakshmi » Unassigned
Status: Active » Needs review
FileSize
1.96 KB
riddhi.addweb’s picture

Status: Needs review » Reviewed & tested by the community

@Munavijayalakshmi, Thanks for patch i checked it with help of coder module in simplytest.me & it cures all the errors after patch applying.PFA

riddhi.addweb’s picture

FileSize
93.74 KB
29.3 KB
guschilds’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for the patch, but it should not be committed as-is. What this patch does is remove empty spaces from the beginning and end of translated strings. This is correct from a code standards point of view, but because these strings are getting appended to other strings, removing those spaces will cause problems in the UI. There will not be spaces between words where there should be. Instead, an alternate approach should be considered, like getting the entire translatable string into one t() call and using variables as needed. More on coding standards with the t() function can be found here. Thanks again.

Gold’s picture

Status: Needs work » Needs review
FileSize
13.02 KB

I've rerolled this patch.

There were a number of other areas that weren't covered in the earlier one. I've included them in this one.

Please note: there are a couple of function variable hints in the migrate file that I had to do a bit of a deep dive into the migrate module to figure out. With there being no tests the only way to know if I got those right will be when someone actually tries to use them. If someone is currently migrating content that uses this module your input/testing of this patch would be greatly appreciated.

This is an aside. If anyone thinks a discussion is needed could they open a separate issue for that? I noticed that everywhere youtube_build_remote_image_path() is used it expects a URL to be returned. Yet, early in the function there is a check that returns an empty string. Looking at where the output of this is used I suspect this empty string will break things. I also suspect that this condition has never occurred so the error has never been seen. Is the check here even needed? I suspect that's a conversation for a different ticket too.

  • guschilds committed 9e1d1bf on 7.x-1.x authored by Gold
    Issue #2862759 by Munavijayalakshmi, Gold, riddhi.addweb, guschilds: Fix...
guschilds’s picture

Status: Needs review » Fixed

Thanks for the patches. I had to re-roll again to work with recent commits, but I've pushed the commit and PHPCS no longer throws any errors for either branch. Thanks again!

Status: Fixed » Closed (fixed)

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