Return newly created item from StoreCrud instead of redirect

[b8ne]

I have had a few instances where I have had to enhance the store method of a certain CRUD controller, requiring the newly created instance to be returned so that its ID can be referenced. The simplest way I have found was to add another case to the return switch in CrudController. I will submit a PR containing that change - based off this issue number. With this method however the specific request input has to be updated to be caught in the switch. I believe my reason for needing this is similar to those mentioned in issue #82 so I dont think there is much point implementing anything further while that is in the pipeline.

[tabacitu]

Hmm... I think we can do that without causing a breaking change by putting the result of the create/update operation inside a variable in $this->crud. Say... $this->crud->entry.

So:

public function storeCrud(StoreRequest $request = null)
    {
        $this->crud->hasAccessOrFail('create');

        // fallback to global request instance
        if (is_null($request)) {
            $request = \Request::instance();
        }

        // replace empty values with NULL, so that it will work with MySQL strict mode on
        foreach ($request->input() as $key => $value) {
            if (empty($value) && $value !== '0') {
                $request->request->set($key, null);
            }
        }

        // insert item in the db
-        $item = $this->crud->create($request->except(['redirect_after_save', '_token']));
+        $this->crud->entry = $this->crud->create($request->except(['redirect_after_save', '_token']));

        // show a success message
        \Alert::success(trans('backpack::crud.insert_success'))->flash();

        // redirect the user where he chose to be redirected
        switch ($request->input('redirect_after_save')) {
            case 'current_item_edit':
-                return \Redirect::to($this->crud->route.'/'.$item->getKey().'/edit');
+               return \Redirect::to($this->crud->route.'/'.$this->crud->entry->getKey().'/edit');

            default:
                return \Redirect::to($request->input('redirect_after_save'));
        }
    }

and

public function updateCrud(UpdateRequest $request = null)
    {
        $this->crud->hasAccessOrFail('update');

        // fallback to global request instance
        if (is_null($request)) {
            $request = \Request::instance();
        }

        // replace empty values with NULL, so that it will work with MySQL strict mode on
        foreach ($request->input() as $key => $value) {
            if (empty($value) && $value !== '0') {
                $request->request->set($key, null);
            }
        }

        // update the row in the db
-       $this->crud->update($request->get($this->crud->model->getKeyName()),
+      $this->crud->entry = $this->crud->update($request->get($this->crud->model->getKeyName()),
                            $request->except('redirect_after_save', '_token'));

        // show a success message
        \Alert::success(trans('backpack::crud.update_success'))->flash();

        return \Redirect::to($this->crud->route);
    }

Then in the EntityCrudController we'd be able to:

public function store(StoreRequest $request)
    {
        // do something before the store event
        $response = parent::storeCrud();
        // do something after the store event, using $this->crud->entry
        return $response;
    }

This would be a pretty nice feature, right?

However, we should take into consideration that "the right way to do before/after creating event" operations would be in the model or in another class, using Laravel Observers. Because in most cases it's something that happens when that entity is created/updated, whether it's done from the CRUD or anywhere else. So in most cases that should not be done in the controller. And inserting this change might promote a bad practice, especially among beginners/intermediates in Laravel.

Thoughts? @OwenMelbz ?

[OwenMelbz]

Oooh to be honest, I've not actually ever used Observables, however after a brief reading it does seem a more organised/modular approach to doing things, even just Events would suffice, https://laravel.com/docs/5.3/eloquent#events so instead of editing the store/update methods you'd so something like

//app/Providers/AppServiceProvider.php
public function boot()
{
    User::created(function ($user) {
        // this will always fire once the user has been created, allowing you to do other things like assign roles etc.
    });
}

[b8ne]

Definitely agree, was just looking for a quick fix to get this project rolling, but would much prefer to implement a better engineered method. So currently my situation is:

ParentEntityCrudController.php

//Add a specific number of child fields for child model entry
for ($i = 0; $i < $number_players; $i++) {
            $this->crud->addField([   // CustomHTML
                'name' => 'position_'.($i + 1),
                'type' => 'position_select',
                'label' => 'Position: '.($i+1),
                'position' => $i+1,
            ]);
        }

position_select.blade.php

//Custom field to input vue component
<?php
    // Logic to fill fields on update
    // Not the most efficient way, however logic is package specific and outside version control
    if (isset($id)) {
        $position = \App\Models\PlayerPosition::where('listId', $id)->where('number', $field['position'])->take(1)->first();
        if($position != null) {
            $field['userId'] = $position->playerId;
            $user = \App\User::find($position->playerId);
            $field['value'] = $user->firstName . ' ' . $user->lastName;
        }
    }

?>

<!-- field_type_name -->
<div @include('crud::inc.field_wrapper_attributes') >
    <label>{!! $field['label'] !!}</label>
    <Typehead name="{{ $field['name'] }}" userid="{{ isset($field['userId']) ? $field['userId'] : '' }}" value="{{ isset($field['value']) ? $field['value'] : '' }}"></Typehead>
</div>

ParentEntityCrudController.php

public function store(StoreRequest $request)
    {
        // First validate to check there isnt already a list for this grade
        $lists = \App\Models\TeamList::where('drawId', $request->request->get('drawId'))->where('grade', $request->request->get('grade'))->get();
        if ($lists->count() > 0) {
            \Alert::error('Unable to create list, a list for this grade already exists.')->flash();
            return \Redirect::to('admin/teamlist');
        }
        // Get Request Params
        $positions = $request->request->get('positions');

        // Now remove positions from request (stock standard CRUD methods dont like it)
        $request->request->remove('positions');

        // Now save parent CRUD to get listId
        $request->request->set('redirect_after_save', 'storeCRUD');
        $list = parent::storeCrud($request);

        // Iterate through positions
        $i = 1;
        foreach($positions as $key) {
            if ($key['id'] != "") {
                // No models on list creation, sp create a new one
                $position = new \App\Models\PlayerPosition([
                    'listId' => $list->listId,
                    'playerId' => $key['id'],
                    'number' => $i
                ]);
                $position->save();
            }
            $i++;
        }

        return \Redirect::to('admin/teamlist');
    }

Hopefully that paints a good enough picture. So based on this, we'd also need a way to pass the child model data in if we used a User::created hook? Is that possible? If so id be more than happy to look into it, ive already learned a hell of a lot by using these packages and am interested in learning more on the inner workings of laraval. Cheers

[b8ne]

Another thought I just had, would it not be possible to add a 'Child Model' trait to the CrudPanel? In which we assign the Child model (namespace). The degree of the relationship can also be stored for use in field model output ie. 0+, 1, 1+ or a specific number. Columns and fields would just need a child flag 'child' => true which is handled in the specific blade template. Then on create/edit/delete, if the crud is a parent, we can handle appropriately. If a child model requires multiple fields, we just handle the request with value['id']['field_1'] etc. I dont see any of this being breaking code either, rather an extension of existing methods.

This is obviously a lot more work than implementing Observers, however even with observers, a lot of custom code would need to be implemented in field templating, relying on the developer.

[OwenMelbz]

Hi @b8ne Yeah It makes more sense now in context :)

I think the steps I would take to try this would be

Firstly you could replace the $field['value'] = $user->firstName . ' ' . $user->lastName; with a mutator so you can use $user->fullName instead of concatenating them each time

//User Model
public function getFullNameAttribute()
{
    return trim( $this->firstName . ' ' . $this->lastName  );
}

Then you have a section that does

// First validate to check there isnt already a list for this grade
        $lists = \App\Models\TeamList::where('drawId', $request->request->get('drawId'))->where('grade', $request->request->get('grade'))->get();
        if ($lists->count() > 0) {
            \Alert::error('Unable to create list, a list for this grade already exists.')->flash();
            return \Redirect::to('admin/teamlist');
        }

This could be moved to the app/Requests/ParentRequest/and handle the validation there (Not ever done this before, so might need some guidance from @tabacitu

Then create a ParentObserver following the instructions https://laravel.com/docs/5.3/eloquent#observers you should end up with a new file within a new Observers folder, loaded in via your app/providers/AppServiceProvider.php file

Then you can hook into the "created" observer which will already have an instance of the $parent, and you can use \Request::get() in there to something like

public function created(Parent $parent){

    $positions = \Request::get('positions');

    $i = 1;
    foreach($positions as $key) {
      if ($key['id'] != "") {
          // No models on list creation, sp create a new one
          $position = new \App\Models\PlayerPosition([
              'listId' => $parent->listId,
              'playerId' => $key['id'],
              'number' => $i
          ]);
          $position->save();
      }
      $i++;
    }
}

So that would be the theory behind it! Completely untested and might need some work, but that should leave the store methods untouched

[b8ne]

Thanks @OwenMelbz that definitely looks like the most straight forward approach. Ill have a tinker this weekend. Ill try to find something that not only solves my problem but is also worthy of a PR

[tabacitu]

Hi @b8ne ,

I completely agree with the suggestions that @OwenMelbz has made. That would be a better way to code this.

However, seeing your store() method has made me think - maaaybe it's not that bad of an idea to let developers code more in the controller. After all:

• it's a non-breaking change;
• it would provide more flexibility;
• sometimes, the operation is not intrinsically linked with the model, so you can't do it in an observer; it's just that you placed more than one entities in the create/update form;
• sometimes, you just need to get shit done fast; and placing all your code in one method instead of 3-4 different files is definitely faster;

So... I say we do it, if you guys can't find a reason why not. What say you? :-)

PS. So sorry, did not understand what you meant about the ChildModel trait on CrudPanel. Could you please rephrase?

[b8ne]

Hey guys, ive just had a bit of a play around, focussing more on something generic rather than me specific need. Sorry i dont really know where to post this kind of thing so ive just committed to a branch on my fork With this, the following would be added to the ControllerStub in Generators:

$this->crud->setChildModel([
            'model' => "App\Models\ChildModel", // Child Model Namespace
            'entity' => 'method', // the method that defines the relationship in your Model
            'multiplicity' => 1 // The number of occurrences of the child - defines number of fields
        ]);

And then this would be a sample field in the controller:

$this->crud->addField([
            'label' => "Number",
            'name' => 'number',
            'type' => 'text',
            'child' => 'App\Models\PlayerPosition'
        ]);

Using this method all existing fields will be useable, and from what I can see there are no breaking changes. Theres still a fair bit more to go in terms of when updating, and also adding further multiplicity support, but figured id get your thoughts before spending any more time on it. Do you think its even worth implementing this? Or will just be better off storing the entity like you said above and get the developer to do the rest?

[OwenMelbz]

Hi guys, this has been addressed in other issues, and there are some PR which address it in the future, so I'm going to close for now :)