egeloen / IvoryCKEditorBundle

Provides a CKEditor integration for your Symfony project.
MIT License
336 stars 114 forks source link

Symfony 4 compatibility #330

Closed mateuszsip closed 5 years ago

mateuszsip commented 6 years ago

Basically #326 with @weaverryan suggestions.

mateuszsip commented 6 years ago

4.0 build failing because composer needs a release.

docker build failing because of missing xdebug.

jbgomond commented 6 years ago

When will this be merged ? Thanks :)

ping @egeloen

jbgomond commented 6 years ago

@kejwmen Services in renderer.xml need to be public too :)

image

mateuszsip commented 6 years ago

I will fix it tomorrow, thanks!

On Sat, 16 Dec 2017, 17:45 Jean-Baptiste Gomond, notifications@github.com wrote:

@kejwmen https://github.com/kejwmen Services in renderer.xml need to be public too :)

[image: image] https://user-images.githubusercontent.com/8344487/34072510-f0192630-e288-11e7-922a-aed847a33669.png

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/egeloen/IvoryCKEditorBundle/pull/330#issuecomment-352194715, or mute the thread https://github.com/notifications/unsubscribe-auth/ABUDM5jSZi2BDzozScL3SjAP8CMoo9Byks5tA_O5gaJpZM4QzifE .

weaverryan commented 6 years ago

Actually, I was a bit wrong when I originally said to make all services public that didn’t previously have a public attribute. In fact, you should only make a service public if it really should be used by the end user. If you don’t add a public attribute, then the user will see a deprecation warning if they try to use it directly from the container.

About the error, if the service in question really doesn’t need to be used directly by the end user, we should leave the public attribute alone and update the library itself to use dependency injection instead of fetching the service directly.

If I’ve made any incorrect assumptions about the library - apologies: I haven’t looked at the code in question :).

mateuszsip commented 6 years ago

@weaverryan you are totally right, I haven't looked at library code and was sure it requires significant amount of work so it would be better to start with simple compat PR.

PR has to wait for composer release and refactoring required to leave renderers private looks straightforward, I will take care of that.

Thanks you found some time to leave this comment ;)

mateuszsip commented 6 years ago

In current state installer script handler won't work with symfony 4 (needs DistributionBundle) and it was the only reason to require composer as a dev-dependency. Looks that composer can be now completely optional and we don't have to wait for new release :tada:

I don't know what @egeloen thinks about it, but at this point I made an assumption that support for symfony < 3.3 and php < 7 can be dropped.

If bundle needs that support some additional work is required, that's why I will wait for opinion.

And I would appreciate a code review, thanks!

laminr commented 6 years ago

any news for that PR ? I am sure that many waits as me to use it in S4 ;-) Thanks for the work

jbgomond commented 6 years ago

ping @egeloen

tjorim commented 6 years ago

For now you can use the following lines in your composer.json to use the fork of @kejwmen :

{
    "require": {
        "egeloen/ckeditor-bundle": "dev-feature/symfony4-compat"
    },
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/kejwmen/IvoryCKEditorBundle"
        }
    ]
}

As @egeloen stated on his Twitter on November 16th, he simply has no time and is looking for maintainers for the Ivory bundles: https://twitter.com/egeloen/status/931240338467049472