Skip to content
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

Remove closing slash as per Joomla Coding Guidelines #16151

Merged
merged 4 commits into from
May 23, 2017
Merged

Remove closing slash as per Joomla Coding Guidelines #16151

merged 4 commits into from
May 23, 2017

Conversation

PhilETaylor
Copy link
Contributor

@PhilETaylor PhilETaylor commented May 20, 2017

Just removing a closing slash from an img tag to make @C-Lodder a happier little boy

Ensure user provided data is correctly encoded.

Ensure user provided data is correctly encoded
@PhilETaylor PhilETaylor reopened this May 20, 2017
$buffer .= '<img src="' . htmlentities($path) . '"' . $class . '>';
$buffer .= sprintf('<img src="%s"%s>',
htmlentities($path, ENT_COMPAT, 'UTF-8', true),
$class );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dedent lines 34 and 35.

@Quy
Copy link
Contributor

Quy commented May 20, 2017

I understand that this was not in the original code. In \plugins\fields\imagelist\tmpl\imagelist.php, directory is accounted for. Does it apply here? If yes, probably should be in a separate PR.

	if (!$path || $path == '-1')
	{
		continue;
	}

	if ($fieldParams->get('directory', '/') !== '/')
	{
		$buffer .= sprintf('<img src="images/%s/%s"%s>',
			$fieldParams->get('directory'),
			htmlentities($path, ENT_COMPAT, 'UTF-8', true),
			$class
		);
	}
	else
	{
		$buffer .= sprintf('<img src="images/%s"%s>',
			htmlentities($path, ENT_COMPAT, 'UTF-8', true),
			$class
		);
	}

@PhilETaylor

This comment was marked as abuse.

@Quy
Copy link
Contributor

Quy commented May 20, 2017

I have tested this item ✅ successfully on a1e0b51


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16151.

@brianteeman
Copy link
Contributor

There is a lot more than just removing the slash in this PR

@N6REJ
Copy link
Contributor

N6REJ commented May 21, 2017

I don't see where the / is added back

@PhilETaylor

This comment was marked as abuse.

@N6REJ
Copy link
Contributor

N6REJ commented May 22, 2017

@PhilETaylor ok, then I'm really confuzzled because then we're breaking html?

@PhilETaylor

This comment was marked as abuse.

@brianteeman
Copy link
Contributor

@N6REJ no we are not. html5 does not require the /

@N6REJ
Copy link
Contributor

N6REJ commented May 22, 2017

@PhilETaylor I think I'll go back to contemplating why the hamster spins the wheel ... thanks

@ghost
Copy link

ghost commented May 23, 2017

With and -out PR got:
bildschirmfoto 2017-05-23 um 08 03 38

System information

3.7.3-dev
Multilanguage Site
macOS Sierra, 10.12.4
Firefox 53 (64-bit)

MAMP 4.1.1

  • PHP 7.0.15
  • MySQLi 5.6.35

@infograf768
Copy link
Member

infograf768 commented May 23, 2017

@franz-wohlkoenig
This works for a media custom field (same now as imagelist).
I get here OK

dl class="fields-container">
			<dd class="field-entry ">
			<span class="field-label">imagelist: </span>
<span class="field-value"><img src="/testnew/trunkgitnew/images/joomla_black.png" class="imgclass"></span>
	</dd>
</dl>

and for media

<dd class="field-entry ">
			<span class="field-label">media: </span>
<span class="field-value"><img src="/testnew/trunkgitnew/images/powered_by.png" class="mediaclass"></span>
	</dd>

@infograf768
Copy link
Member

I have tested this item ✅ successfully on a1e0b51


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16151.

@PhilETaylor

This comment was marked as abuse.

@ghost
Copy link

ghost commented May 23, 2017

RTC after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 23, 2017
@ghost
Copy link

ghost commented May 23, 2017

@infograf768 didn't saw its about com_fields.

@brianteeman brianteeman modified the milestone: Joomla 3.7.3 May 23, 2017
@rdeutz rdeutz merged commit 595e50a into joomla:staging May 23, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants