search

phpcr / phpcr-utils

A set of helper classes and command line commands you want to use with phpcr, but that are not part of the API itself
phpcr.github.io
Other
72 stars 30 forks source link

fix php 7.1 string to array conversion issue #162

Closed ahilles107 closed 8 years ago

ahilles107 commented 8 years ago

PHP 7.1 have BC change

    The empty index operator (e.g. $str[] = $x) is not supported for strings
    anymore, and throws a fatal error instead of silently converting to array.

This bug is the only one which breaks our tests in php 7.1

ahilles107 commented 8 years ago

I tried to add php 7 and 7.1 to travis but looks likes there is to advanced for me stuff there. You can find errors here: https://travis-ci.org/phpcr/phpcr-utils/builds/163440029.

But this fix solves my issue, and is good practice (don't change type of variable in fly) - and pass all existing tests on php<7.0.

ahilles107 commented 8 years ago

@dbu @richard-fizz - can you take a look?

dbu commented 8 years ago

thanks! travis fails with php 7. some things fail with mocked objects. can you check that? did you not get that locally? i guess we don't care about the phpunit warning - we want to also support older php versions where we can't use a version that has createMock.

ahilles107 commented 8 years ago

Like i wrote in first comment - is to advanced for me. Have no idea why Node is not referenceable. But i get those same errors on php7 locally without fix. So it's not connected to it.

shinyvision commented 8 years ago

It seems like whatever the ValueConverterTest is testing, it is using a node that doesn't have a path value (either null or an empty string). If you take a look at PHPCR\Util\ValueConverter:146 you'll see it should have provided a node path in the exception which it didn't actually do.

I don't have time to actively fix this problem, but I hope this helps anyone in their search :)

ahilles107 commented 8 years ago

@dbu can we merge just this thing and fix PHP7 tests in another one?

dbu commented 8 years ago

i continued in #163

dbu commented 8 years ago

@richard-fizz the error must be somewhere around there. however, that the node has no path should not matter - the problem is the line above with the mocked isNoteType which seems to not return the expected result. we mock the node in ValueConverterTest on line 55. i fixed isNodeType to be correct upper/lowercase but that did not help.