Skip to content

Conversation

@rullzer
Copy link
Member

@rullzer rullzer commented Jul 21, 2016

This means that a controller inherriting from the ApiController and not
implementing their own constructor would fail.

  • Unit tests updated

Found this while helping @schiessle debug.
CC: @BernhardPosselt @nickvergessen @MorrisJobke @LukasReschke

@rullzer rullzer added enhancement 3. to review Waiting for reviews labels Jul 21, 2016
@rullzer rullzer added this to the Nextcloud 11.0 milestone Jul 21, 2016
@mention-bot
Copy link

@rullzer, thanks for your PR! By analyzing the annotation information on this pull request, we identified @BernhardPosselt, @Xenopathic and @DeepDiver1975 to be potential reviewers

@rullzer
Copy link
Member Author

rullzer commented Jul 21, 2016

$parameters[] = $this->query($resolveName);
try {
$parameters[] = $this->query($resolveName);
continue;
Copy link
Member

Choose a reason for hiding this comment

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

Why not simply put the next try catch into the catch block? continue is super confusing here

@BernhardPosselt
Copy link
Member

Actually unsure if this is a good idea since adding default parameters will make the container always ignore them.

I'd probably make the parent constructor protected since the class is abstract anyways, that would require you to call the parent method and prevent these mistakes. Unfortunately there's no operator overloading in PHP.

Another approach would be to also define these defaults in the container so it actually works

@rullzer
Copy link
Member Author

rullzer commented Jul 22, 2016

Another approach would be to also define these defaults in the container so it actually works

Oeee I like that....
Let me change this PR.

@rullzer
Copy link
Member Author

rullzer commented Jul 22, 2016

I'd probably make the parent constructor protected since the class is abstract anyways, that would require you to call the parent method and prevent these mistakes. Unfortunately there's no operator overloading in PHP.

We can't really do that. because if we want to make the OCSControllers constructor protected we have to make the APIControllers constructor private and then would mean making the Controllers constrcutor private.

@rullzer rullzer force-pushed the appframework_magic_allow_default_vars branch from a787fc7 to 1b73a63 Compare July 22, 2016 08:12
@rullzer
Copy link
Member Author

rullzer commented Jul 22, 2016

@BernhardPosselt this looks cleaner indeed

@nickvergessen
Copy link
Member

We can't really do that. because if we want to make the OCSControllers constructor protected we have to make the APIControllers constructor private and then would mean making the Controllers constrcutor private.

Not really, you can make protected methods public on extends: https://3v4l.org/NL1gv

@rullzer
Copy link
Member Author

rullzer commented Jul 22, 2016

@nickvergessen yes but it is the other way around https://3v4l.org/qHXuO

@rullzer rullzer changed the title AppFramework buildClass ignored default values AppFramework add default values (ApiController) as parameters Jul 22, 2016
@MorrisJobke
Copy link
Member

@rullzer Could you explain what this does and how to verify it works?

@nickvergessen
Copy link
Member

Could you explain what this does and how to verify it works?

  1. Create a controller, and do not add the constructor.
  2. Use automatic DI (server->query('Class')) to get an instance
  3. See how it (not) throws

@nickvergessen
Copy link
Member

👍

1 similar comment
@MorrisJobke
Copy link
Member

👍

@MorrisJobke MorrisJobke merged commit e51afa1 into master Jul 25, 2016
@MorrisJobke MorrisJobke deleted the appframework_magic_allow_default_vars branch July 25, 2016 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants