Follow-up to #2454393: Upgrade to Symfony 2.6.5 & #2414235: Upgrade to Symfony 2.6.4 & #2377281: Upgrade to Symfony 2.6 stable & #2464369: Upgrade to Symfony 2.6.6.

Symfony 2.7.1 was released on 11th of June 2015.

Have a skim of the issue summary on #2454393: Upgrade to Symfony 2.6.5 for a better overview of why upgrading point releases is a good idea :).

This has no security fixes, but full changelog below.

Changelog changelog.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it is an external library upgrade.
Issue priority Major because this is a minor external library upgrade.
Disruption Not disruptive.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, Symfony-2.7.1.patch, failed testing.

jibran’s picture

Try git diff --binary

The last submitted patch, Symfony-2.7.1.patch, failed testing.

joshtaylor’s picture

FileSize
252.26 KB

With git diff --binary

joshtaylor’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: 2504967-1.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
249.15 KB

Let me try.

Status: Needs review » Needs work

The last submitted patch, 7: upgrade_to_symfony_2_7_1-2504967-7.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
252.6 KB

I am giving this a try. It might be a temporary issue. Anyway, it seems like there were missing .gitignore files. I am not sure if my run of composer update is adding that somehow or if it is actually new. But I am attempting to test again anyway and see what the testbot says.

Also, the problem seems to be with hiddeninput.exe but it doesn't look like it has changed in last 3 years. There was similar discussion in #2498515: Update additional Symfony Components to 2.7.0 but it was resolved there by modifying .gitattributes file. I see that the change is already there in the .gitattributes file.

Status: Needs review » Needs work

The last submitted patch, 9: upgrade_to_symfony_2_7_1-2504967-9.patch.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
237.65 KB

This seems to be a bug in PIFR. There is code which checks for \r in new lines and if found, it will throw an error.

It finds it in this patch, but that is actually part of the hiddeninput.exe file, which is required. I am not sure how to fix this.

To test if this is indeed the case, I am attaching a new patch after removing hiddeninput.exe from the index. I am guessing this patch should work.

Another point of interest: I tried to run hiddeninput.exe from Drupal's repo on my Windows machine (in cygwin, but still...) and it said it was bad exec format. The .exe I got after running composer update ran fine. I am thinking the exe in Drupal is still not valid. I would suggest that it should be placed during commit, if possible.

Let's see what the testbot says.

Status: Needs review » Needs work

The last submitted patch, 11: upgrade_to_symfony_2_7_1-2504967-11.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
236.09 KB

Okay, it failed because there is another file in the patch with \r line endings, and that too is a binary file: core/vendor/symfony/dependency-injection/Tests/Fixtures/includes/ProjectWithXsdExtensionInPhar.phar. It is a new file in the repo.

I have removed that too from the patch. I am guessing tests may fail, but lets see if it at least moves forward this time. I am not sure if phar would care if these are CR-LF or just LF.

hussainweb’s picture

Green! I see Drupal tests passed, of course, but Symfony tests would probably fail.

Since this is green, a workaround is that a maintainer would just run composer update symfony/* and commit. That would close this issue. I don't know if that will happen. Thoughts?

Status: Needs review » Needs work

The last submitted patch, 13: upgrade_to_symfony_2_7_1-2504967-13.patch, failed testing.

hussainweb’s picture

I am rerunning the composer update command instead of rerolling. I have included the patch file as in #9 (which might fail again for the same reasons) and a files-removed patch as in #13. Same notes as #14 apply for that patch.

I am also raising it to critical because this is now potentially blocking a critical issue. See: #2493911-93: Update guzzle, goutte and mink-goutte-driver to the latest release.

The last submitted patch, 17: upgrade_to_symfony_2_7_1-2504967-17.patch, failed testing.

hussainweb’s picture

I see it still fails. The full explanation is in #11 if you're interested. Also, read my comment at #14 for a workaround (but a maintainer has to do that).

Berdir’s picture

Status: Needs review » Needs work

hiddeninput.exe was not changed since it was added in 2012. It's just git in some configuration that's messing it up. We can just ignore that.

Not sure about the new file, but we've ignored files from symfony before that conflict with git/testbot. Have a look at core/.gitignore. I think some of them were due to php 5.4 syntax, and we could actually add them now.

Anyway, I suggest you manually revert changes to hiddeninput.exe and add the new .phar to that exclusion list. Then I think we're good to go?

Also, would be good to know if this *really* is unblocking the other issue :) Should be easy to enough by making a combined patch?

hussainweb’s picture

@Berdir: hiddeninput.exe has not changed in the source but what we have right now is not valid. I tried running this on my machine and it gave an error saying that it was an invalid format. It is a good idea to update it with the actual file, isn't it? It is not .gitignore because I see the differences. I think the last commit changed \r\n in it to just \n, which made it invalid.

I will check the gitignore file now.

Berdir’s picture

Yes, it might be invalid but it doesn't matter. There is no need to block this update on fixing that problem, let's open a new issue on testbot so that it doesn't do the newline check on binary files. Maybe the new testbot doesn't do that at all (yet).

hussainweb’s picture

@Berdir: In that case, the files-removed.patch is good to go. That is the same patch as before with those two files removed. Should this be back to needs review then?

Berdir’s picture

No, it's missing the .gitignore change to explicitly exclude the new file (for now) so that other won't fall in the same trap next time we update.

alexpott’s picture

The hiddeninput problem has been dealt with before I think - yep - see #2498515: Update additional Symfony Components to 2.7.0 and the changes to .gitattributes

hussainweb’s picture

@alexpott: If you see the patch in #2498515-12: Update additional Symfony Components to 2.7.0, it contains hiddeninput.exe, when it shouldn't have been. As @Berdir pointed out, that file hasn't changed since 2012. This was later noted in comment #13 of that issue as well. I guess the patch passed in that case because somehow, git changed the newlines from \r\n to \n in the binary file. I would suggest either remove the file, or fix it. But I agree that we could do that in a follow-up instead of holding a critical.

I will post the patch after removing this file for now and adding the phar file in the gitignore. We could at least go ahead with that. I guess there should be an issue in PIFR for this as well.

hussainweb’s picture

I have created a followup at #2531796: Fix hiddeninput.exe in symfony/console. I have also attached a patch for changes to .gitignore as mentioned in #24. This is the only new block in .gitignore (for quick review here):

# The PHAR file below contains CRLF characters that cause a problem with PIFR.
vendor/symfony/dependency-injection/Tests/Fixtures/includes/ProjectWithXsdExtensionInPhar.phar

I have not kept hiddeninput.exe in this and putting it in the followup.

hussainweb’s picture

FYI, I also submitted an issue to PIFR at #2531798: Handle binary files in patches.

hussainweb’s picture

@alexpott: I have submitted a patch in #2531796: Fix hiddeninput.exe in symfony/console to fix the hiddeninput.exe issue. Since it is quite old file, it could be done without depending on this issue. It would be great if you could consider that issue along with this one as well. It is a separate issue so as to not tie up the critical, but I think it is strongly related.

Also, should we leave a comment in the .gitignore file to remove the ignore line for phar once the issue in PIFR is fixed - #2531798: Handle binary files in patches?

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Everything is addressed. We have follow ups. We have green patch so RTBC. Thanks @hussainweb for sticking with it.

dawehner’s picture

Yeah, let's fix the hiddeninput.exe problem in the follow up!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed b65b472 and pushed to 8.0.x. Thanks!

  • alexpott committed b65b472 on 8.0.x
    Issue #2504967 by hussainweb, joshtaylor, jibran, Berdir: Upgrade to...
neclimdul’s picture

Status: Fixed » Closed (fixed)

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