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

[3.4] Allow for consecutive prefills. #6919

Merged
merged 5 commits into from
Aug 21, 2017

Conversation

bobdenotter
Copy link
Member

@bobdenotter bobdenotter commented Aug 20, 2017

  • Allow for consecutive prefills, with the Lorem Ipsum generator
  • Fix logic error in $form->get('contenttypes')->has('contenttypes')
  • Small UI change: show the "prefill with Lorem Ipsum" button not only after finished DB check.

Fixes #6917.

@@ -197,15 +197,17 @@ public function prefill(Request $request)

if ($request->isMethod('POST') || $request->query->getBoolean('force')) {
$form->handleRequest($request);
if ($form->get('contenttypes')->has('contenttypes')) {
if (!empty($form->get('contenttypes')->getData())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Friends don't let friends use empty()

Copy link
Member Author

Choose a reason for hiding this comment

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

It returns [] when nothing is selected. Open to suggestions on how to do it properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

The negation will work for an empty array, however after running though xdebug it returns an [0] which won't pass negation, but checking Symfony docs when the field type is ChoiceType and multiple is true, and expand is true it should return an empty array … that isn't empty to me … so for now your empty() wins as a solution to getting this over the line. Just unsure as to what that line is.

@GwendolenLynch
Copy link
Contributor

So we are or we are not allowing this? That is why the force was there.

@bobdenotter
Copy link
Member Author

The 'force' is for a slightly different usecase. If you're on a fresh install with no content, it shows the blue bar:

screen shot 2017-08-20 at 14 54 51

This link has the ?force=1 attached to it. What it does is not "overriding" the number of items to prefill, but rather skip the screen where it asks which contenttypes to prefill, but instead going straight ahead to prefilling all the empty ones.

@bobdenotter bobdenotter force-pushed the hotfix/allow-consecutive-prefills branch from 48ff97b to 440a71b Compare August 20, 2017 13:18
@bobdenotter bobdenotter changed the title [3.4] Allow for consecutive prefills. [3.4] [WIP] Allow for consecutive prefills. Aug 20, 2017
*
* @return null
*/
public function build(array $contentTypeNames, $count)
public function build(array $contentTypeNames, $count, $skipNonEmpty)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a BC break (as shown by the tests)

@GwendolenLynch
Copy link
Contributor

Given your coming back online from a couple of weeks away, RL work is going to want all of your attention, I'll pick this up and get it over the line for you.

@bobdenotter
Copy link
Member Author

I though about cheating, and changing it to:

public function build(array $contentTypeNames, $count, $skipNonEmpty = true)

, but that would obviously not be the right way to go.

If you have time, fixing it would be much appreciated. If not, i'll make time in the next few days. :-)

@GwendolenLynch
Copy link
Contributor

GwendolenLynch commented Aug 21, 2017

Yeah, I am making time … there is a legitimate problem to be fixed here, this PR goes most of the way to fixing it, and I really don't want to delay this beta any longer than needed.

bobdenotter and others added 5 commits August 21, 2017 10:17
- Use a counter for each ContentType loop
- Rename variable and set a default to not break BC
- Simplify logic and don't double-tap the get()
- Kick the logic out of the nest … time to fly
- Invert logic
- Comment logic on of the approach
@GwendolenLynch GwendolenLynch force-pushed the hotfix/allow-consecutive-prefills branch from 440a71b to 4bb508f Compare August 21, 2017 08:18
@GwendolenLynch
Copy link
Contributor

There we go … passes tests, and no BC breaks.

@GwendolenLynch GwendolenLynch changed the title [3.4] [WIP] Allow for consecutive prefills. [3.4] Allow for consecutive prefills. Aug 21, 2017
@GwendolenLynch GwendolenLynch merged commit 239ff50 into 3.4 Aug 21, 2017
@GwendolenLynch GwendolenLynch deleted the hotfix/allow-consecutive-prefills branch August 21, 2017 08:33
@GwendolenLynch GwendolenLynch modified the milestone: Bolt 3.4 - Feature release Aug 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants