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

Correct use of render function for Symfony greater than 2.6 #188

Merged
merged 5 commits into from
Aug 2, 2016
Merged

Correct use of render function for Symfony greater than 2.6 #188

merged 5 commits into from
Aug 2, 2016

Conversation

jeantristan
Copy link
Contributor

@jeantristan jeantristan commented Aug 2, 2016

I'm targeting branch 3.x cause it's a bugfix.
However it's not BC compatible with all symfony2 versions.

Fixes : No open issues.
(Do i have to open an issue to make a reference ?)

Changelog

### Fixed
- call of render function now Sf3 compatible

Subject

Change from a render tag to a render function

@greg0ire
Copy link
Contributor

greg0ire commented Aug 2, 2016

Please don't just ignore the PR template, it's one of the main problem with the #182.

@@ -51,11 +51,11 @@ file that was distributed with this source code.
<span id="field_actions_{{ id }}" class="field-actions">
<span id="field_widget_{{ id }}" class="field-short-description">
{% if sonata_admin.field_description.associationadmin.id(sonata_admin.value) %}
{% render url('sonata_admin_short_object_information', {
{% render(url('sonata_admin_short_object_information', {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't use display tags like in #182, I'm afraid this won't do anything.

@@ -51,12 +51,12 @@ file that was distributed with this source code.
<span id="field_actions_{{ id }}" class="field-actions">
<span id="field_widget_{{ id }}" class="field-short-description">
{% if sonata_admin.field_description.associationadmin.id(sonata_admin.value) %}
{% render url('sonata_admin_short_object_information', {
{{ render (url('sonata_admin_short_object_information', {
Copy link
Contributor

Choose a reason for hiding this comment

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

No space between the function name and the parenthesis please.

@greg0ire
Copy link
Contributor

greg0ire commented Aug 2, 2016

Do you know what I'm talking about when I say PR template ? It is the piece of text that was in the textarea when you submitted the PR. You can still edit your PR to fill it, here is what the template looks like : https://raw.githubusercontent.com/sonata-project/SonataDoctrineMongoDBAdminBundle/3.x/.github/PULL_REQUEST_TEMPLATE.md

@greg0ire
Copy link
Contributor

greg0ire commented Aug 2, 2016

Also, some repos have a CONTRIBUTING.md, and link to it appears when you submit your PR. Make sure you read ours

@jeantristan
Copy link
Contributor Author

Ok thanks. I updated my PR using PR template.
I have some questions:

  • I don't know if it's BC compatible or not ?
  • Do i have to open an issue to reference it in the PR ?
  • Do i have to update the tests?

@greg0ire
Copy link
Contributor

greg0ire commented Aug 2, 2016

Ok thanks. I updated my PR using PR template.

It's better.

  • This is backwards compatible IMO : the render() function exists in 2.3
  • Do i have to open an issue to reference it in the PR ? No
  • Do i have to update the tests? No you don't.

@greg0ire
Copy link
Contributor

greg0ire commented Aug 2, 2016

LGTM I added some comments, so you have to fix them first, and then your commit message has to be changed, either by the person who will merge this, or by yourself (git commit --amend, git push --force).

Here is what I suggest :

Replace deprecated render tag with function

@greg0ire
Copy link
Contributor

greg0ire commented Aug 2, 2016

So to sum up, here is your todo list :

@@ -55,8 +55,7 @@ file that was distributed with this source code.
'code': sonata_admin.field_description.associationadmin.code,
'objectId': sonata_admin.field_description.associationadmin.id(sonata_admin.value),
'uniqid': sonata_admin.field_description.associationadmin.uniqid
})
)}}
})) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Almost there : indent this to the left once and it will be perfect!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you tell me, how am I supposed to know it?
I did not read it in PSR-2 / PSR-1 or Symfony coding standards.
It's just to avoid to make any new mistake

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not php, so nothing to do with the PSR, but indention means that just like you indent content inside a hash, like this :

{% set my hash = {
    'item1',
    'item2',
    'item3'
} %}

you have to do it no matter where the hash is (in our case, inside a function call).

@jeantristan
Copy link
Contributor Author

@greg0ire
Copy link
Contributor

greg0ire commented Aug 2, 2016

In order to do it, do i have to do another PR? Sorry to bother, I don't know the process.

Oh I see what you did. You need to learn how to rebase (see CONTRIBUTING.md), so that you can produce one commit with the right message.

But don't bother, just fix this, I'll take care of making one commit with the right message, I can do it when merging.

@greg0ire
Copy link
Contributor

greg0ire commented Aug 2, 2016

Finally good! I'll merge as soon as the build is ok, thanks a lot!

@jeantristan
Copy link
Contributor Author

Next time, I'll do better ;-)

@greg0ire greg0ire merged commit 7b3a9f7 into sonata-project:3.x Aug 2, 2016
@greg0ire
Copy link
Contributor

greg0ire commented Aug 2, 2016

Thanks @jeantristan !

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

Successfully merging this pull request may close these issues.

4 participants