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

Add symfony 3.0 support #122

Closed
wants to merge 10 commits into from
Closed

Add symfony 3.0 support #122

wants to merge 10 commits into from

Conversation

GuilhemN
Copy link

This PR adds symfony 3.0 support and removes the usage of the deprecated FlattenException class.

Fix #115, #121, #116, #73

@GuilhemN
Copy link
Author

The build fail is not related to this PR.
(see https://travis-ci.org/schmittjoh/JMSJobQueueBundle/jobs/97841916)

@temp
Copy link
Contributor

temp commented Dec 30, 2015

I've got the same error with travis in my PR #70.

"doctrine/common": ">=2.3,<3.0",
"jms/di-extra-bundle": ">=1.1,<2.0"
"php": "^5.5|^7.0",
"symfony/framework-bundle": "^2.3|^3.0",
Copy link
Owner

Choose a reason for hiding this comment

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

2.1 should still work afaics, no? any reason why we need 2.3 here?

Copy link
Author

Choose a reason for hiding this comment

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

Symfony 2.1 and 2.2 are not supported anymore so IMO we should not continue support them too.

Copy link
Owner

Choose a reason for hiding this comment

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

If we don't need the higher version, let's keep the old 2.1 constraint.

Copy link
Author

Choose a reason for hiding this comment

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

Changed

@GuilhemN
Copy link
Author

@temp the tests error message really need to be improved... :/

@whyte624
Copy link

Waiting for this pull request to be pulled. 👍

@GuilhemN
Copy link
Author

@whyte624 unfortunately I don't know how to fix the tests :-/

@whyte624
Copy link

@Ener-Getick right now I'm using your SF3 branch.
So could you please fix kernel.root_dir in commands?
For example, jms/job-queue-bundle/Command/RunCommand.php:433
change $this->getContainer()->getParameter('kernel.root_dir') to something which returns bin directory.

@GuilhemN
Copy link
Author

@whyte624 the RunCommand now uses a Finder to find the console command ;-)

@e-doceo
Copy link

e-doceo commented Jan 12, 2016

+1

1 similar comment
@khasinski
Copy link
Contributor

👍

@ruslan-polutsygan
Copy link

any news when this can be merged?

@@ -428,9 +429,25 @@ private function getCommandProcessBuilder()
$pb->add('exec');
}

// Localize the `console` command
Copy link
Owner

Choose a reason for hiding this comment

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

Can we detect the console command based on how this command itself was started?

Copy link
Author

Choose a reason for hiding this comment

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

How would you do that?

Copy link
Owner

Choose a reason for hiding this comment

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

If you check the content of $_SERVER, there should be everything you need.

Copy link
Author

Choose a reason for hiding this comment

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

This looks much more complicated as the user can use either php console or console so we would have to parse the input.

Copy link
Contributor

Choose a reason for hiding this comment

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

realpath($_SERVER['SCRIPT_FILENAME'])
// or
realpath($_SERVER['argv'][0])

should do the trick, whether the php executable is mentioned or not.

This looks safer than expecting to find a single file called console somewhere under the root dir - which is a convention that not all projects might follow.

Copy link
Author

@GuilhemN GuilhemN Jul 31, 2016

Choose a reason for hiding this comment

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

@nclavaud could you send a PR on my repo ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

@nclavaud i don't think it will work when using phpunit :/

@ruslan-polutsygan
Copy link

@Ener-Getick
can you merge master in your branch, or make rebase?
this is fixed in master but still an issue in your branch #123

@GuilhemN
Copy link
Author

done @ruslan-polutsygan

@jiri-jagos
Copy link

May I ask when approximately is this going to be merged into the master please?

@dextervip
Copy link

+1

@stephanvierkant
Copy link
Contributor

stephanvierkant commented Jun 8, 2016

Any news on this? What are we waiting for?

@dextervip
Copy link

It looks like repo owner is not reviewing issues and pull requests anymore.

@GuilhemN
Copy link
Author

GuilhemN commented Jun 8, 2016

@stephanvierkant @dextervip you can still try to send him a message on twitter, he may have too many notifications ;-)

@dextervip
Copy link

@Ener-Getick Are you using this pull request in your production env with sf3?

@GuilhemN
Copy link
Author

GuilhemN commented Jun 8, 2016

@dextervip no i don't, but it seems that others do (my repo has been forked several times and some issues have been reported).

@dextervip
Copy link

There is one test failing in travis build, Any ideas? I guess It should be fixed before we hit up

@GuilhemN
Copy link
Author

GuilhemN commented Jun 8, 2016

@dextervip in fact this is already failing in the current state and the test pass on my machine so i have no idea from what it comes.

@dextervip
Copy link

@Ener-Getick Yes, just noticed it.

Maybe we should ask to be created a 1.x branch to maintain legacy bundle support and use master to develop new sf3 support, new BC features and release 2.x versions. What do you think guys?

@GuilhemN
Copy link
Author

@dextervip this PR is fully bc so i don't think we need to release a new major version.

@nullziu
Copy link

nullziu commented Aug 2, 2016

Eagerly waiting for this to get merged! 👍

@deeravenger
Copy link

Still waiting.. 😉

@lordjancso
Copy link

Is there any news about this?

@cmmata
Copy link

cmmata commented Sep 7, 2016

@lordjancso @dmkuznetsov @nullziu If you want a workaround while this request is not accepted, I have been using @Ener-Getick's fork since June 22 without any problem.

@GuilhemN
Copy link
Author

It looks like @schmittjoh added sf 3 support in the latest commits, so let's close this pr :)

@GuilhemN GuilhemN closed this Sep 17, 2016
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.