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

elgg_format_element() object to string conversion #10010

Closed
hypeJunction opened this Issue Jul 20, 2016 · 8 comments

Comments

Projects
None yet
2 participants
@hypeJunction
Contributor

hypeJunction commented Jul 20, 2016

Passing an array of objects to elgg_format_element() causes issues at:

if (is_array($val)) {
   $val = implode(' ', $val);
}

@hypeJunction hypeJunction changed the title from elgg_format_element() array to string conversion to elgg_format_element() object to string conversion Jul 20, 2016

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Jul 20, 2016

Member

Passing an array of objects

Why should the function support that?

Member

mrclay commented Jul 20, 2016

Passing an array of objects

Why should the function support that?

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Jul 20, 2016

Contributor

It shouldn't, but it tries to convert objects to strings.
We need to filter the array values before trying to concatenate them:

elgg_format_attributes([
   'items' => [
     new ElggFile(),
     new ElggObject(),
   ]
]);
if (is_array($val)) {
   $val = array_filter($val, function($e) {
      return is_scalar($e) || (is_object($e) && is_callable([$e, '__toString']));
   });
   $val = implode(' ', $val);
}
Contributor

hypeJunction commented Jul 20, 2016

It shouldn't, but it tries to convert objects to strings.
We need to filter the array values before trying to concatenate them:

elgg_format_attributes([
   'items' => [
     new ElggFile(),
     new ElggObject(),
   ]
]);
if (is_array($val)) {
   $val = array_filter($val, function($e) {
      return is_scalar($e) || (is_object($e) && is_callable([$e, '__toString']));
   });
   $val = implode(' ', $val);
}
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Jul 20, 2016

Member

I'd rather just throw if a non-scalar is present than to filter.

Member

mrclay commented Jul 20, 2016

I'd rather just throw if a non-scalar is present than to filter.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Jul 20, 2016

Contributor

Throwing will cause issues with $vars. We can just skip if array contains non-scalar values

Contributor

hypeJunction commented Jul 20, 2016

Throwing will cause issues with $vars. We can just skip if array contains non-scalar values

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Jul 20, 2016

Contributor

What if:

elgg_format_attributes([
   'class' => [
      'elgg-foo',
      ['elgg-bar'],
      'elgg-baz',
   ],
]);
Contributor

hypeJunction commented Jul 20, 2016

What if:

elgg_format_attributes([
   'class' => [
      'elgg-foo',
      ['elgg-bar'],
      'elgg-baz',
   ],
]);
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Jul 20, 2016

Member

😿 . This function can't be the Universal Serializer. Ignore non scalars and trigger an error?

Member

mrclay commented Jul 20, 2016

😿 . This function can't be the Universal Serializer. Ignore non scalars and trigger an error?

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Jul 20, 2016

Contributor

It triggers a php warning. I think we should just skip quietly. We inject
$vars a lot and that contains all kinds of data

On Jul 20, 2016 4:40 PM, "Steve Clay" notifications@github.com wrote:

😿 . This function can't be the Universal Serializer. Ignore non scalars
and trigger an error?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#10010 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABJaSVgl_ZQ7GjVu37fRVVWb8PnHgHyPks5qXjN2gaJpZM4JQx4E
.

Contributor

hypeJunction commented Jul 20, 2016

It triggers a php warning. I think we should just skip quietly. We inject
$vars a lot and that contains all kinds of data

On Jul 20, 2016 4:40 PM, "Steve Clay" notifications@github.com wrote:

😿 . This function can't be the Universal Serializer. Ignore non scalars
and trigger an error?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#10010 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABJaSVgl_ZQ7GjVu37fRVVWb8PnHgHyPks5qXjN2gaJpZM4JQx4E
.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Jul 22, 2016

Member

Fixed by #10013

Member

mrclay commented Jul 22, 2016

Fixed by #10013

@mrclay mrclay closed this Jul 22, 2016

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