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

Added support for `classes` function #2798

Open
wants to merge 1 commit into
base: 2.x
from

Conversation

Projects
None yet
4 participants
@lyrixx
Copy link
Contributor

lyrixx commented Jan 9, 2019

fixes #2797

@lyrixx lyrixx force-pushed the lyrixx:classes branch from 3f4549b to 4c679e1 Jan 9, 2019

@yceruto
Copy link
Contributor

yceruto left a comment

Shouldn't this be implemented for 1.x branch first?

} elseif (is_array($arg)) {
foreach ($arg as $class => $condition) {
if (!is_string($class)) {
throw new Twig_Error_Runtime(sprintf('Key #%d of argument #%d should be a string. Got "%s".', $class, $i, gettype($class)));

This comment has been minimized.

@yceruto

yceruto Jan 9, 2019

Contributor

For 2.x branch use RuntimeError instead (Same below)

This comment has been minimized.

@yceruto

yceruto Jan 9, 2019

Contributor

Nevermind, this class doesn't use the 2.x namespace anyway.

@@ -0,0 +1,12 @@
``classes``
============

This comment has been minimized.

@yceruto

yceruto Jan 9, 2019

Contributor

extra =

} elseif (is_array($arg)) {
foreach ($arg as $class => $condition) {
if (!is_string($class)) {
throw new Twig_Error_Runtime(sprintf('Key #%d of argument #%d should be a string. Got "%s".', $class, $i, gettype($class)));

This comment has been minimized.

@stof

stof Jan 9, 2019

Contributor

Saying Key #2 makes me think it is the 3rd item in the array (as we count from 0). But array(2 => true) would defeat that. I think you should remove the # here.

$classes = [];
foreach ($args as $i => $arg) {
if (is_string($arg)) {

This comment has been minimized.

@stof

stof Jan 9, 2019

Contributor

should we allow a Twig_Markup object here, by casting it ?

This comment has been minimized.

@lyrixx

lyrixx Jan 10, 2019

Contributor

I guess yes :) I will update the PR

@lyrixx

This comment has been minimized.

Copy link
Contributor

lyrixx commented Jan 10, 2019

What should be the base branch?

@stof

This comment has been minimized.

Copy link
Contributor

stof commented Jan 10, 2019

I think it should be 1.x. AFAIK, 1.x still gets minor version releases.

@fabpot

This comment has been minimized.

Copy link
Contributor

fabpot commented Jan 12, 2019

IIUC, it's purely related to HTML. If that's the case, I don't think it belongs to the core.

@fabpot

This comment has been minimized.

Copy link
Contributor

fabpot commented Jan 12, 2019

Might be more appropriate in Twig-extensions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment