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

FormatterType Support SF3 #146

Conversation

rafaelcalleja
Copy link

add to #145 sonata_formatter_type support in SF3

@rafaelcalleja rafaelcalleja force-pushed the PR_format_type_supp_sf3 branch 6 times, most recently from dad649a to d812edd Compare February 24, 2016 13:25
@OskarStark
Copy link
Member

LGTM

@greg0ire
Copy link
Contributor

And the commit adds tests… great work!

{
parent::tearDown();

$this->extension = null;
Copy link
Member

Choose a reason for hiding this comment

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

IMO, we don't need this

Copy link

Choose a reason for hiding this comment

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

done

@rafaelcalleja rafaelcalleja force-pushed the PR_format_type_supp_sf3 branch 2 times, most recently from 5969e51 to e04575b Compare February 25, 2016 13:09
@dnoise
Copy link

dnoise commented Feb 25, 2016

I've upgraded this PR with master because the Travis CI was failing but the upgrade doesn't fix the Test.

Thank to the failed test I guess have detected a issue about versions of egeloen/ckeditor-bundle from the last merge #110 .

Too In my project i can see:

The service "sonata.formatter.twig.ck_editor" has a dependency on a non-existent service "ivory_ck_editor.templating.helper".

The service ivory_ck_editor.templating.helper was renamed in version 2.4 but composer.json is allowing version 2.2
"egeloen/ckeditor-bundle": "^2.2|^3.0"

You can check it from this commit

To fix it and the test too simply need change composer.json to

"egeloen/ckeditor-bundle": "~2.4|^3.0",

maybe this change would be in a new PR or Issue?

@greg0ire
Copy link
Contributor

maybe this change would be in a new PR or Issue?

I think it would. Can you do it? @Th3Mouk can you comment on this?

@dnoise
Copy link

dnoise commented Feb 25, 2016

The firt version of egeloen/ckeditor-bundle with support for symfony/form:~3.0 is 4.0

I guess we can support sf3 with something like this
"egeloen/ckeditor-bundle": "~2.4|^3.0|~4.0"

@Th3Mouk
Copy link
Contributor

Th3Mouk commented Feb 25, 2016

I've started to develop the upgrade in october and there's not 4.x releases, i agree that the composer.json must be upgrade from ^2.2 to ^2.5 because when i've tested i was in 2.5.2.

Il will make the upgrade of composer.

The ^4.0 upgrade seems really simple, @dnoise would you do the PR ?

@Th3Mouk
Copy link
Contributor

Th3Mouk commented Feb 25, 2016

I've check it's ok for 4.x support there no BC, i've made PR to #148

@OskarStark
Copy link
Member

OskarStark commented Apr 25, 2016

can we merge this @Th3Mouk ?

@Th3Mouk
Copy link
Contributor

Th3Mouk commented Apr 25, 2016

@OskarStark I can't pronounce on the testing part, that must be extract on a base class apparently.

@rafaelcalleja could you rebase please ? Tests are red actually

@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@soullivaneuh
Copy link
Member

According to the new Sonata version management and next major release plan, this project has been refactored regarding branching and versioning.

If you see this message, your PR concerns a patch or a minor release and is not targeting the right branch.

So I'm closing this one, but don't see it as a refusal. If you think your work is still relevant and want to continue, feel free to reopen it on the right branch (e.g. the default one).

Regards.

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

Successfully merging this pull request may close these issues.

8 participants