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.

Sign up for GitHub

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

Jump to bottom

Restore backwards-compatibility for render calls #4772

Closed
wants to merge 2 commits into from
Closed

Restore backwards-compatibility for render calls #4772

wants to merge 2 commits into from

Conversation

greg0ire
Copy link
Contributor

@greg0ire greg0ire commented Nov 20, 2017 •
edited
Loading

I am targeting this branch, because this is BC.

Close sonata-project/SonataPageBundle#910

Changelog

### Deprecated
- Calling CRUDController::render

### Fixed
- BC-break in `render` behavior

To do

  • test the deprecation
  • Add an upgrade note

@greg0ire greg0ire mentioned this pull request Nov 20, 2017
Closed
soullivaneuh
soullivaneuh reviewed Nov 20, 2017
View reviewed changes
src/Controller/CRUDController.php
$method
));
}
@trigger_error(sprintf(
Copy link
Member

Choose a reason for hiding this comment

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

You should avoid sprintf for performance, use string concatenation instead.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer readability over performance here

@greg0ire
Copy link
Contributor Author

Argh, this is not so simple, one of the methods is relying on is_callable

Grégoire Paris and others added 2 commits November 20, 2017 23:07
@greg0ire greg0ire mentioned this pull request Nov 20, 2017
Closed
@greg0ire
Copy link
Contributor Author

Argh again, __call is not called in 2.8 because the method was public back them :(

@greg0ire
Copy link
Contributor Author

I'm a bit out of ideas now... how to support several signatures, one of which is public?

@greg0ire
Copy link
Contributor Author

I think the right call is to just revert my previous PR and deal with that later... by dropping sf 2 ?

@jordisala1991
Copy link
Member

jordisala1991 commented Nov 20, 2017 •
edited
Loading

If you try to override a method that is final, does it give an exception even if you don't call it?

@greg0ire
Copy link
Contributor Author

? It's not final...

@jordisala1991
Copy link
Member

I do think we need time to think how to do this, might be a good idea to just revert your last PR to get some time

@greg0ire
Copy link
Contributor Author

yup this is definitely not an easy one. Closing for now.

@greg0ire greg0ire closed this Nov 20, 2017
@greg0ire greg0ire deleted the restore_bc branch November 20, 2017 22:55
@greg0ire greg0ire mentioned this pull request Nov 22, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soullivaneuh soullivaneuh soullivaneuh left review comments

@core23 core23 core23 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@greg0ire @jordisala1991 @soullivaneuh @core23