Fix CRUDController::editAction ignoring its optional parameter #3961

curry684 · 2016-06-17T12:40:17Z

I am targetting this branch, because it's broken in the current branch. Will have to be forward merged into master.

The CRUDController::editAction accepted an $id parameter which was always overwritten, serving 99% of use cases just fine, but not the exceptional case it was added for.

Changelog

### Fixed
 - Made CRUDController::editAction respect optional parameter

greg0ire · 2016-06-17T12:53:06Z

Please keep your commit message subject under 50 chars, as described in the CONTRIBUTING.md file. You can tell us more in the § below it.

greg0ire · 2016-06-17T12:54:11Z

Controller/CRUDController.php

@@ -234,7 +234,7 @@ public function editAction($id = null)
        // the key used to lookup the template
        $templateKey = 'edit';

-        $id = $request->get($this->admin->getIdParameter());
+        $id = is_null($id) ? $request->get($this->admin->getIdParameter()) : $id;


Can you please also document this behavior in the @param annotation ?

Why not:

$id = $id ?: $request->get($this->admin->getIdParameter());

soullivaneuh · 2016-06-17T13:05:14Z

Thank you for your contribution. 👍

Please next time do not remove the code block around the change-log content. This is wanted to have this. 😉

Could you also please reduce a bit your commit title? It's very long.

curry684 · 2016-06-17T13:19:25Z

Please keep your commit message subject under 50 chars, as described in the CONTRIBUTING.md

With all due respect, I did read the CONTRIBUTING.md and halfway through considered not making the PR at all because making a PR shouldn't be an hour of work for a 30 second fix. The number of rules and requirements in there discourage busy people like me from fixing small but serious issues like this one. I fully understand the need for a project this large to have proper rules, but the amount of work requested from a contributor should be in line with the size of the PR. I dearly recommend adding a big "Ignore the 1000 rules below for basic and obvious bug fixes" on top of the PR template.

I didn't use ?: btw because I thought it was PHP 5.4+ hehe.

greg0ire · 2016-06-17T13:29:00Z

Well, the 50 chars rule is not really specific to our project. It's something that you should know if you use git regulary, so much that even github softly enforces it :

ProTip! Great commit summaries are 50 characters or less. Place extra information in the extended description.

Also, if you use vim to write your commit message, you will see the commit message color change if you break this rule (and others).

I understand that you are busy though, so if you don't have the time to do better, we will simply squash merge and change the commit message for you.

Finally, I know the CONTRIBUTING.md file is long, but I think it is really full of sound advice. Please do read it, and do read the linked articles.

greg0ire · 2016-06-17T13:29:37Z

Your PR now has 99 commits… use git rebase -i origin/3.x to fix that.

soullivaneuh · 2016-06-17T13:30:47Z

With all due respect, I did read the CONTRIBUTING.md and halfway through considered not making the PR at all because making a PR shouldn't be an hour of work for a 30 second fix. The number of rules and requirements in there discourage busy people like me from fixing small but serious issues like this one. I fully understand the need for a project this large to have proper rules, but the amount of work requested from a contributor should be in line with the size of the PR

I understand your point of view but we can't do another way. It's already hard to maintain 30 sonata projects on out free time, so we don't have the time to try to understand a maybe malformed PR and ask people to update it.

I dearly recommend adding a big "Ignore the 1000 rules below for basic and obvious bug fixes" on top of the PR template.

👎 for two reasons:

The basic and/or obvious level defined by what? The use pushing the PR? In this case we have no limit
Some part as mandatory even for a small PR, like the changelog for example.

I didn't use ?: btw because I thought it was PHP 5.4+ hehe.

Don't blame PHP 5.3. It cans do some things sometime. 😛

greg0ire · 2016-06-17T13:37:15Z

Controller/CRUDController.php

@@ -221,7 +221,7 @@ public function deleteAction($id)
    /**
     * Edit action.
     *
-     * @param int|string|null $id
+     * @param int|string|null $id Optional ID to use instead of globally provided default.


curry684 · 2016-06-17T13:39:11Z

I was squashing my own commits and accidentally rebased on master hehe, fixed now.

As for the contributing - I'm just pointing out it's now damaging the project that contributions are discouraged by the amount of administrative work required. I have in the past on other projects (Symfony and Doctrine most notably) not submitted serious fixes for the same reason, I have paying customers who do appreciate me spending 5 minutes to fix an upstream issue fundamentally, but not spending 2 hours reading contribution guidelines. If that's needed I'll just work around the issue locally, and I'm not the only one reasoning like that.

I fully agree for features and invasive changes you need proper argumentation and documentation, I just think (know) it's terribly counterproductive for the project quality to do it for oneliner fixes like this where something is beyond obviously broken.

greg0ire · 2016-06-17T13:41:40Z

LGTM! Thanks for taking time on your busy schedule to fix that :) Our guidelines are quite verbose indeed, and our rules are quite strict, but we try to compensate that by being reactive, so time you "loose" making the PR cleaner should hopefully be forgotten when seeing the PR quickly merged. This workflow works for us and I don't think we will change it : see how the pulse is steady and contributions keep coming despite all that.

soullivaneuh · 2016-06-17T13:43:48Z

I have paying customers who do appreciate me spending 5 minutes to fix an upstream issue fundamentally, but not spending 2 hours reading contribution guidelines. If that's needed I'll just work around the issue locall, and I'm not the only one reasoning like that.

Your point of view, this is again understandable.

But please try to understand our: We don't have lot of time, time is important.

If someone don't want to add a PR because of that, that's bad but we won't change our rules for that.

I'm pretty sure Symfony have those kind of rules for common reasons, despite of having more resources and time than us.

soullivaneuh · 2016-06-17T13:44:29Z

RTM. Thank you @curry684! 👍

OskarStark · 2016-06-18T04:50:32Z

Good Job 👍🏻

SonataCI added the review required label Jun 17, 2016

greg0ire reviewed Jun 17, 2016
View reviewed changes

curry684 force-pushed the 3.x branch from 0b2d5de to ca6760b Compare June 17, 2016 13:28

soullivaneuh added pending author patch and removed review required labels Jun 17, 2016

CRUDController::editAction now respects $id param

4815b7a

curry684 force-pushed the 3.x branch from ca6760b to 4815b7a Compare June 17, 2016 13:33

SonataCI added review required and removed pending author labels Jun 17, 2016

greg0ire reviewed Jun 17, 2016
View reviewed changes

greg0ire added RTM and removed review required labels Jun 17, 2016

greg0ire merged commit be82d59 into sonata-project:3.x Jun 17, 2016

soullivaneuh removed the RTM label Jun 18, 2016

This was referenced Jun 21, 2016

Do not use provided ID on editAction if concerning the child #3965

Closed

Revert "Fix CRUDController::editAction ignoring its optional parameter" #3969

Merged

curry684 mentioned this pull request Jun 24, 2016

Decide what to do with unused id parameter #3970

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix CRUDController::editAction ignoring its optional parameter #3961

Fix CRUDController::editAction ignoring its optional parameter #3961

curry684 commented Jun 17, 2016 •

edited by soullivaneuh

Loading

greg0ire commented Jun 17, 2016

greg0ire Jun 17, 2016

soullivaneuh Jun 17, 2016 •

edited

Loading

soullivaneuh commented Jun 17, 2016

curry684 commented Jun 17, 2016 •

edited

Loading

greg0ire commented Jun 17, 2016

greg0ire commented Jun 17, 2016 •

edited

Loading

soullivaneuh commented Jun 17, 2016 •

edited

Loading

greg0ire Jun 17, 2016

curry684 commented Jun 17, 2016 •

edited

Loading

greg0ire commented Jun 17, 2016

soullivaneuh commented Jun 17, 2016

soullivaneuh commented Jun 17, 2016

OskarStark commented Jun 18, 2016

Fix CRUDController::editAction ignoring its optional parameter #3961

Fix CRUDController::editAction ignoring its optional parameter #3961

Conversation

curry684 commented Jun 17, 2016 • edited by soullivaneuh Loading

Changelog

greg0ire commented Jun 17, 2016

greg0ire Jun 17, 2016

Choose a reason for hiding this comment

soullivaneuh Jun 17, 2016 • edited Loading

Choose a reason for hiding this comment

soullivaneuh commented Jun 17, 2016

curry684 commented Jun 17, 2016 • edited Loading

greg0ire commented Jun 17, 2016

greg0ire commented Jun 17, 2016 • edited Loading

soullivaneuh commented Jun 17, 2016 • edited Loading

greg0ire Jun 17, 2016

Choose a reason for hiding this comment

curry684 commented Jun 17, 2016 • edited Loading

greg0ire commented Jun 17, 2016

soullivaneuh commented Jun 17, 2016

soullivaneuh commented Jun 17, 2016

OskarStark commented Jun 18, 2016

curry684 commented Jun 17, 2016 •

edited by soullivaneuh

Loading

soullivaneuh Jun 17, 2016 •

edited

Loading

curry684 commented Jun 17, 2016 •

edited

Loading

greg0ire commented Jun 17, 2016 •

edited

Loading

soullivaneuh commented Jun 17, 2016 •

edited

Loading

curry684 commented Jun 17, 2016 •

edited

Loading