gremo / ZurbInkBundle

Creating email templates is hard. This Symfony Bundle provides help.
MIT License
24 stars 12 forks source link

Allow Symfony 5 #8

Closed signor-pedro closed 2 years ago

signor-pedro commented 4 years ago

This is work-in-progress.

So far, it seems these are the only necessary changes.

kernel.root_dir is no longer available, you have to use kernel.project_dir which is present since Symfony 3.3 (should be enough backward-compatibility).

signor-pedro commented 4 years ago

I checked your composer.json and it seems this package still supports SF2.8.

Would you suggest a runtime check for kernel.root_dir parameter presence, or would you release this as a new version with bumped compatibility version?

Sth like

arguments: ["@=container.hasParameter('some_param') ? parameter('some_param') : 'default_value'"]

(https://symfony.com/doc/current/service_container/expression_language.html)

gremo commented 4 years ago

Hi, thanks! SO have you tested it, all works just fine? If yes, please squash all commits into 1 and I will merge into master and release.

gremo commented 4 years ago

Ping @signor-pedro ?

signor-pedro commented 4 years ago

Ah sorry, will do squashing tomorrow. :)

signor-pedro commented 4 years ago

Hi, sorry, I completely forgot.

The commits are squashed now, however since you do not have tests, I cannot guarantee this 100% works, it seems to me everything is normal.

If you could at least checkout dev-master in one of your projects and do a quick sanity-check, that would be great.

Also, please note this upgrade makes it SF2-incompatible.

gremo commented 4 years ago

Hi, thanks for your contribution!

Why you removed the ^2.8 version requirement in composer.json? Also, removing kernel.root_dir is something we can't do.

signor-pedro commented 4 years ago

Hi, I explained my reasons in the upper comment where I also proposed a solution with conditionally binding arguments.

Let me know if you are OK with the conditional in services.yaml before I work on an update. Thanks.

signor-pedro commented 4 years ago

Essentially I would do this in services.xml

<argument type="expression">
    container.hasParameter('project_dir') ? parameter('project_dir') : parameter('root_dir')
</argument>
gremo commented 4 years ago

Yep I'm fine with conditionals in services.xml, but I don't think that they are supported in Symfony 2.8 (have you tried?).

So, the only way to solve this is to change GremoZurbInkExtension (marking rootDir a private variable) or use a compiler pass to change that argument (I prefer this way). Are you able to do that?

As an anternative, to you have a Symfony 5 test/demo repository I can download, for testing the (modded) bundle?

signor-pedro commented 4 years ago

I think I should be able to do the compiler pass, thanks for the feedback!

rgeraads commented 4 years ago

any news on this?

gremo commented 4 years ago

So sorry for the late response. I'm currently quite busy at work and more JavaScript oriented.

Anyways, since the bundle doesn't provide any test, are we sure that all changes (classes, parameters) will not break something? Are we dropping 2.8 compatibility, right?

dbollaer commented 3 years ago

any update?

ruzampl commented 3 years ago

also waiting @signor-pedro @gremo

hracik commented 2 years ago

So sorry for the late response. I'm currently quite busy at work and more JavaScript oriented.

Anyways, since the bundle doesn't provide any test, are we sure that all changes (classes, parameters) will not break something? Are we dropping 2.8 compatibility, right?

I think you should accept it, if something is broken it will be fixed once it goes live and naturally become more tested.

About dropping SF2.8 to me it is completely fine, anybody who use it can use current and not the latest version.

I will be able to make some basic tests in mine project and help with solutions if something does not work.

hracik commented 2 years ago

I just found out that even this pull request is bit outdated at the moment since it doesn't support PHP 8 in composer.json which is standard for new projects. So more work needs to be done, but let's start with something.

gremo commented 2 years ago

Done, can @hracik test master version and see if all it's fine?

hracik commented 2 years ago

I tested it and made a new pull request with some changes, please check and let me know if you don't like something.