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

[Templating] [Twig] Sf3.0 support : Changing render from tag to function #154

Closed
wants to merge 4 commits into from
Closed

Conversation

Id4v
Copy link

@Id4v Id4v commented Dec 18, 2015

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets none
License MIT
Doc PR -

Since Sf3.0 Twig render tag became a function.
Updated some templates that were using render as a tag.

@Id4v
Copy link
Author

Id4v commented Jan 27, 2016

Thanks to @zajca Tests are all green now.
(Dependencies to security-acl has been added)

Ping @rande or anyone from the team.

"doctrine/mongodb-odm-bundle": "~3.0",
"doctrine/mongodb-odm": "~1.0",
"sonata-project/admin-bundle": "~2.3"
"doctrine/mongodb-odm-bundle": "~3.0@dev",
Copy link
Contributor

Choose a reason for hiding this comment

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

It shouldn't depend of dev dependencies (same below).

Copy link

Choose a reason for hiding this comment

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

In that case, there should be some releases of this bundle or some develop branch since it's not great also in projects depend on master of this bundle.

@@ -18,8 +18,8 @@
],
"require": {
"doctrine/mongodb-odm-bundle": "~3.0",
"doctrine/mongodb-odm": "~1.0",
"sonata-project/admin-bundle": "~2.3"
"sonata-project/admin-bundle": "~2.3.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

~2.3 ?

Copy link

Choose a reason for hiding this comment

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

Well this will fail with symfony3, I would expect that master branch will support symfony3.
Also this template change should break symfony <2.6

Copy link
Author

Choose a reason for hiding this comment

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

~2.3 will result in fetching dev-master or 2.4.x-dev which are the same.

Anyway, if not fetching 2.4.x-dev travis tests on 3.0 result in an error because SonataAdmin still uses Symfony\Component\Validator\ValidatorInterface; in 2.3.x instead of Symfony\Component\Validator\Validator\ValidatorInterface;.

So either we target a dev version of SonataAdminBundle or we wait for 2.4.x-dev to release a stable version.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO the best option is to wait for a new stable release of SonataAdminBundle.

Copy link

Choose a reason for hiding this comment

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

😓

@SonataCI
Copy link
Collaborator

SonataCI commented Jun 4, 2016

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.

@jeantristan
Copy link
Contributor

The issue still happens with:
"sonata-project/admin-bundle": "dev-master",
"sonata-project/doctrine-mongodb-admin-bundle": "dev-master",

Is there a way to work around the issue ?

@greg0ire
Copy link
Contributor

See #182

@jeantristan
Copy link
Contributor

If i'm understanding correctly, someone has to do the PR? Right?

@greg0ire
Copy link
Contributor

greg0ire commented Aug 2, 2016

It's more about cleaning the existing PR. Would you like to do it?

@jeantristan
Copy link
Contributor

Yeah sure.
I did another PR cause i don't know if it's possible to change another PR.
Check #188

@greg0ire
Copy link
Contributor

greg0ire commented Aug 2, 2016

I did another PR cause i don't know if it's possible to change another PR.

Creating another PR is the only way I know of. You can't access #182 unless the authors gives you right on his repo.

@jeantristan
Copy link
Contributor

Ok, tell me if I did the right thing.
If i'm wrong, tell me, it's my first PR

@greg0ire
Copy link
Contributor

greg0ire commented Aug 2, 2016

Ok, tell me if I did the right thing.
If i'm wrong, tell me, it's my first PR

The main problem with your PR is that you completely removed the PR template when submitting it. Was it intentional?

@jeantristan
Copy link
Contributor

I updated it. Hope it's better.

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