Autosave not working on content creation

We could not offer any autosave for content that doesn't exist. So you have first to create and save the content and afterwards during editing it autosave states will be created. Does this answer your question?

I had the same question when I installed this module today.

I now understand that the entity has to be created first before autosave will work.

So, is there an easy way to give the user a link to "create a node" that automatically creates a node and then redirects the user to the edit page (instead of going to node/add/contenttype, where autosave doesn't work)?

So, is there an easy way to give the user a link to "create a node" that automatically creates a node and then redirects the user to the edit page (instead of going to node/add/contenttype, where autosave doesn't work)?

No, this is not something that belongs in the autosave module.

When I initially implemented the module, I was thinking that we need an entity ID and when the entity is not yet saved there is no entity ID. But I think that in such cases we could use as an entity ID just 0 which will identity new entities. Do you wann give it a try with a patch and I could take a look at it?

@hchonov I agree that the "automatic node creation/redirect to edit page" is out of scope for this module, but I was wondering if there was a guide for doing this somewhere. If not, I will try to figure it out and then write up instructions, which perhaps could be included as a code example in the module's README file.

I think it's better to only allow autosave on created entities (entities with an ID), because using an EID of 0 may be dangerous since many users may each go to the /node/add/contenttype page for multiple content types, and it seems like having a bunch of content with no EID could lead to a security issue. So instead, I want to find a way to save the entity and then edit it on creation.

Working on a solution here: https://drupal.stackexchange.com/questions/251788/link-to-create-a-node-...

Will need work but it seems to do the job.

I need help in "autosave_form_form_alter" with the proper route detection.

Thank you. This looks promising :).

1. +++ b/autosave_form.module
   @@ -100,3 +100,33 @@ function autosave_form_entity_update(EntityInterface $entity) {
   + * Implements hook_entity_insert().
   ...
   +function autosave_form_entity_insert(EntityInterface $entity) {
   

   Looks like copy paste of autosave_form_entity_update. It would be better to provide a helper method and call it from the the insert and update hook.

2. +++ b/autosave_form.module
   @@ -100,3 +100,33 @@ function autosave_form_entity_update(EntityInterface $entity) {
   +        $autosave_entity_form_storage->purgeAutosavedEntityState($entity->getEntityTypeId(), $entity->id(), $autosave_session_id, NULL, NULL, $uid);
   

   We should use here 0 as well for the entity ID, but better would be to call a method returning the entity ID to use.

3. +++ b/src/Form/AutosaveEntityFormHandler.php
   @@ -320,4 +320,19 @@ class AutosaveEntityFormHandler implements AutosaveEntityFormHandlerInterface, E
   +   * We use 0 for the values of a new entity.
   +   *
   +   * @param $entity
   +   * @return int
   

   A better documentation is needed :).

4. +++ b/src/Form/AutosaveEntityFormHandler.php
   @@ -320,4 +320,19 @@ class AutosaveEntityFormHandler implements AutosaveEntityFormHandlerInterface, E
   +  protected function getEntityIdOrZero($entity) {
   

   The method name could be simply getEntityId().

5. +++ b/src/Form/AutosaveEntityFormHandler.php
   @@ -320,4 +320,19 @@ class AutosaveEntityFormHandler implements AutosaveEntityFormHandlerInterface, E
   +    if ($entity->isNew()) {
   +      return 0;
   +    }
   +    else {
   +      return $entity->id();
   +    }
   

   This could be changed to return $entity->isNew() ? 0 : $entity->id().

6. +++ b/src/Form/AutosaveFormBuilder.php
   @@ -170,7 +170,12 @@ class AutosaveFormBuilder extends FormBuilder {
   +        if ($entity->isNew()) {
   +          $entity_id = 0;
   +        } else {
   +          $entity_id = $entity->id();
   +        }
   
   +++ b/src/Storage/AutosaveEntityFormDatabaseStorage.php
   @@ -194,8 +194,14 @@ class AutosaveEntityFormDatabaseStorage implements AutosaveEntityFormStorageInte
   +      if ($entity->isNew()) {
   +        $entity_id = 0;
   +      } else {
   +        $entity_id = $entity->id();
   +      }
   

   As we do it at multiple places we need a method for retrieving the entity ID from everywhere. We could make the method defined in the AutosaveEntityFormHandler as public static and use it here.

+++ b/autosave_form.module
@@ -52,10 +52,10 @@ function autosave_form_form_alter(&$form, FormStateInterface $form_state, $form_
+  if ($route && ($route_defaults = $route->getDefaults()) && (isset($route_defaults['_entity_form']) || isset($route_defaults['_controller']))) {
...
+    if ((($entity->getEntityTypeId() != $entity_type_id) || ($form_object->getOperation() != $form_op)) && !isset($route_defaults['_controller'])) {

The comment above states why we restrict the autosave only to routes which are defined through _entity_form. If we check for _controller on the route we don't now if this is the form being retrieved by that controller. I guess that what is causing problems here is the node add route? Then probably you should open an issue in core to exchange the controller through an _entity_form definition in the options :).

Thanks for reviewing. I also found that i missed an "isNew()" check in the "storeEntityAndFormState".

I apologize for the lack of interdiff. I screwed something up with branches and commits :) I think i figured it out now.

1. They are not 100% copy pasted. With insert we only purge the store for the specific user. My thinking was that multiple people can work on a new node at the same time and if one saves then all could potentially lose their work. Do you still think we should make a helper method?
2. Can entity ID ever be 0 when you update?
3. I hope its better now :)
4. Done
5. Done
6. Done
7. #9 I am sorry but that surpasses my knowledge. I wouldn't even know how to explain what i want :) So i think its best that you open an issue if you think it is required.

re-rolled patch against latest dev

Would it be reasonable to adjust the autosave_form_entity_form table to save an entity uuid rather than the entity id?

If that was switched, the node/add/{bundle} handler could redirect to node/add/{bundle}/{uuid}, autosave_form could start autosaving this new, unique, item and the node add form could use this new uuid when creating a node if it exists in the route.

Items in autosave_form_entity_form could be cleaned up on cron if no entity exists to match the uuid.

I think using 0 like this means users will only be able to work on one unique unsaved node, so if they open node/add several times and work on many separate items they'll be sort of randomly overwriting the autosave data, no?

In the meantime I may be using the 'redirect node/add to node/edit on a new node' to have autosave always active, but cleaning up blank nodes is a bit problematic there.

Had a go at adjusting your patch @MaskOta and rather than using 0 for the id, using the uuid if it exists.

I think the uuid will always exist? New nodes are created but not saved and they generate a uuid so that's good. I'm not certain of entities that do not use uuids but if there are none we can ditch ids all-together, yeah? This patch just adds length to the id field to acommodate uuids or ids. Probably should make a new uuid field or switch id to uuid.

I added a route override for node/add/{type} that goes to node/add/{type}/{uuid}. Might be unnecessary but will allow users to get back to their in progress node from browser history. I think without that it's not useful?

I've only tried making nodes so I might be missing something for other entities like config.

Thanks!

Latest patch works well.

Re #12:

Would it be reasonable to adjust the autosave_form_entity_form table to save an entity uuid rather than the entity id?

It would be nice to create a dedicated issues for such questions. But still an answer - no, I don't think that it makes any sense to use the uuid instead the entity ID. First of all the autosave states are local and shared among environments. And also this would make the current issue impossible to solve, because new entities always have a different uuid.

Items in autosave_form_entity_form could be cleaned up on cron if no entity exists to match the uuid.

Why adding such a complexity when we already have the entity hooks taking care of this?

I think using 0 like this means users will only be able to work on one unique unsaved node, so if they open node/add several times and work on many separate items they'll be sort of randomly overwriting the autosave data, no?

There is an issue to prevent 2 autosave sessions - #3038789: Prevent overwriting fields with second browser tab/window.

In the meantime I may be using the 'redirect node/add to node/edit on a new node' to have autosave always active, but cleaning up blank nodes is a bit problematic there.

This is not a good idea, because the forms on the add and edit pages might be different as well as the form alter hooks might be manipulating the form only for one of them.

In general I don't really see any advantage of using UUIDs instead of IDs. However if you still feel like this might be useful, then please open a new issue and let's continue the discussion there.

Re #9:
Unfortunately we cannot have that change. See the consequences of it in #3083756: Undefined index and Undefined offset $route_defaults['_entity_form'].

Re #10:

1. They are not 100% copy pasted. With insert we only purge the store for the specific user. My thinking was that multiple people can work on a new node at the same time and if one saves then all could potentially lose their work. Do you still think we should make a helper method?

   There is still very big overlap between hook_entity_update and hook_entity_insert with the only difference that in the latter we consider the current user. This all could be put in a helper method having an optional user parameter, which is invoked by both hook implementations.

2. Can entity ID ever be 0 when you update?

   No, 0 should be passed to the helper function only from within the insert hook.

3. I hope its better now :)

   Unfortunately not. I'll comment on that again in a next comment.

I tried taking out the two references to $route_defaults['_controller'] mentioned in #9 and that were causing the conditions to allow the code to get to $route_defaults['_entity_form'] even if that was not set, causing the errors:

Notice: Undefined offset: 1 in autosave_form_form_alter() (line 78 of /app/web/modules/contrib/autosave_form/autosave_form.module) #0 /app/web/core/includes

Notice: Undefined index: _entity_form in autosave_form_form_alter() (line 78

it did stop the errors from showing in the recent error log .... but also, when I revisited an existing node edit page that had been locked and autosaving, when answering to resume editing, the changes I had made were not preserved.

(Note I did not try on node add.)

So I agree this still needs work.

There was enough difference that the automatic patch apply could not apply the patch, re applied manually, and it did apply with a lot of offsets. Here is just that reroll with no changes.

(setting to needs review, just to see if the d.o test bot can apply it)

oops. the patch command left .orig files. sorry. here it is without that cruft.

and here is the change I tried to see if it might still work without the _controller in the ifs.

Crosspost, a review based on the previous patch.

1. +++ b/autosave_form.module
   @@ -74,10 +74,10 @@ function autosave_form_form_alter(&$form, FormStateInterface $form_state, $form_
   +  if ($route && ($route_defaults = $route->getDefaults()) && (isset($route_defaults['_entity_form']) || isset($route_defaults['_controller']))) {
   ...
   +    if ((($entity->getEntityTypeId() != $entity_type_id) || ($form_object->getOperation() != $form_op)) && !isset($route_defaults['_controller'])) {
   

   As already written in the previous comment this change causes problems.

   It was introduced because of how the node add route is defined:

   node.add:
     path: '/node/add/{node_type}'
     defaults:
       _controller: '\Drupal\node\Controller\NodeController::add'
   

   However this could be changed in core and let core define the route through _entity_form instead. See also the recommendation in entity.api.php from the Entity routes section:

   For entity form routes, use _entity_form rather than the generic _controller or _form.

   The problem with relying on _controller is that we have to ensure that there is only a single entity form shown. I haven't yet thought about how we could do this, but as soon as this is against the recommendation of defining entity form routes, I don't think that it is worth it considering this and introducing such complexity at all. Instead we should open a core issue for switching from _controller to _entity_form for the node.add route. I've created an issue and provided a patch for that - #3084030: route node.add should be defined through _entity_form instead through _controller. It would be nice if someone could review it.

2. +++ b/autosave_form.module
   @@ -122,3 +122,33 @@ function autosave_form_entity_update(EntityInterface $entity) {
   +function autosave_form_entity_insert(EntityInterface $entity) {
   ...
   +      // If conflict is enabled and the entity is saved then delete only the
   +      // current autosave session of the current user.
   +      if ($autosave_session_id = $class::getAutosaveSessionID($entity)) {
   +        $autosave_entity_form_storage->purgeAutosavedEntityState($entity->getEntityTypeId(), $entity->id(), $autosave_session_id, NULL, NULL, $uid);
   +      }
   

   While I was thinking about how to unify both the insert and the update functions, I've realized that the insert function only needs this lines and all the others are unnecessary. The reason is that we when we insert an entity we don't care whether conflict is enabled or not and we simply want to delete the autosave states for the current autosave session. Also we should not retrieve the ID for that through $entity->id(), as the newly saved entity will already have a real ID assigned, but for the autosave states we used as identifier 0.

3. +++ b/src/Form/AutosaveEntityFormHandler.php
   @@ -320,4 +320,14 @@ class AutosaveEntityFormHandler implements AutosaveEntityFormHandlerInterface, E
   +  /**
   +   * Return the entity ID or 0 if the entity is new and no ID has been set yet.
   +   *
   

   Let's put the extended documentation as a note. I suggest the following method documentation:

   /**
    * Returns the entity ID to use for the autosave storage.
    *
    * We cannot use the entity ID in all cases. For example new entities do not 
    * yet have an entity ID, but we still need to return something.
    * 
   

4. +++ b/src/Form/AutosaveEntityFormHandler.php
   @@ -320,4 +320,14 @@ class AutosaveEntityFormHandler implements AutosaveEntityFormHandlerInterface, E
   +   * @param $entity
   ...
   +  public static function getEntityId($entity) {
   

   Needs type hint and documentation.

5. +++ b/src/Form/AutosaveEntityFormHandler.php
   @@ -320,4 +320,14 @@ class AutosaveEntityFormHandler implements AutosaveEntityFormHandlerInterface, E
   +   * @return int
   

   Missing new line between the parameter and return declarations. The return type should be

   int|string<code>.
   </li>
   
   <li>
   <code>
   +++ b/src/Form/AutosaveEntityFormHandler.php
   @@ -320,4 +320,14 @@ class AutosaveEntityFormHandler implements AutosaveEntityFormHandlerInterface, E
   +   *   Entity ID or 0 for new entities.
   

   "The entity ID to use for the autosave storage."

6. +++ b/src/Form/AutosaveFormBuilder.php
   @@ -170,7 +170,8 @@ class AutosaveFormBuilder extends FormBuilder {
   +        $entity_id = AutosaveEntityFormHandler::getEntityId($entity);
   
   +++ b/src/Storage/AutosaveEntityFormDatabaseStorage.php
   @@ -194,8 +193,9 @@ class AutosaveEntityFormDatabaseStorage implements AutosaveEntityFormStorageInte
   +      $entity_id = AutosaveEntityFormHandler::getEntityId($entity);
   

   As the autosave handler might be exchanged we need to retrieve the class from the entity type instead of using the default class.

7. +++ b/src/Form/AutosaveEntityFormHandler.php
   @@ -326,4 +326,14 @@ class AutosaveEntityFormHandler implements AutosaveEntityFormHandlerInterface, E
   +    return $entity->isNew() ? 0 : $entity->id();
   

   I've also just realized, that we have one drawback of having the getEntityId method checking for the entity being new - we cannot use it in the insert hook, as the entity isn't new anymore. Having a method and hardcoding 0 at other places is not really acceptable. We need to come up with a solution, which we might use everywhere.

Re #23:

and here is the change I tried to see if it might still work without the _controller in the ifs.

@YesCT, the change is okay for autosave (we just need to still completely revert it and remove the surrounding brackets). However node.add will not work anymore. I've created an issue and provided a patch for that - #3084030: route node.add should be defined through _entity_form instead through _controller. It would be nice if someone could review it.

Let's make it configurable per setting whether Autosave should be allowed for new entities and check for that setting in AutosaveEntityFormHandlerInterface::isAutosaveEnabled().

I am not yet sure whether the feature should be enabled or disabled by default. Any thoughts on that?

The referenced issue was committed to Drupal 8.8.x, which means that there is no need for the _controller change here in order for autosave to work on node creation. Yay!

Then we have to decide if "autosave on node creation" is only supported in 8.8.x, or do we want to build some fallback for 8.7?

Either we try to convince a committer to commit the issue or part of it to 8.7 as well or we include here a fallback and allow the node.add route.

Hi everybody, first thank you for this module ! it is a really good feature for contributor.

Also i wanted to use the module with the version 8.x-1.0 and not the dev branch.
And the drupal core is 8.7.
The patch from #23 wasn't working, but i added the "if" on the controller :
"if ($route && ($route_defaults = $route->getDefaults()) && (isset($route_defaults['_entity_form']) || isset($route_defaults['_controller']))) {"

Then it was working but with 2 warning on $route_defaults and $entity_type_id, so i add a check on them and now it is working for drupal 8.7 and autosave_form 8.x-1.0.

Here is the patch for the people who needed it.

The patch #30 works fine for me. Thanks!

Answering comments from #25.
#1 Since node add uses an entity form route, we are fine. Just removed extra brackets.
#2 Rewritten insert hook, please check. Do we need checks for entity autosave_form handler? How it is possible that there isn't one if we define default in hook_entity_info_alter?
#3,4,5 Updated documentation and code style
#6 Used the handler with the checks if it exists, which (see #2) I'm not sure are necessary.
#7 I don't have a good idea how to solve this. Bad ideas are setting some flag on entity in insert hook, that might change the outcome of getEntityId() method, or using another hook, eg, presave.

Do we need checks for entity autosave_form handler? How it is possible that there isn't one if we define default in hook_entity_info_alter?

If it was unset explicitly in a following hook implementation.

1. +++ b/autosave_form.module
   @@ -144,3 +144,24 @@ function autosave_form_user_update(EntityInterface $user) {
   +  $autosave_session_id = ($entity_type->hasHandlerClass('autosave_form'))
   +    && ($class = $entity_type->getHandlerClass('autosave_form'))
   

   It will be easier to understand and read if we simply skip the whole processing if there is no autosave_form handler. Lets add a constant and document ::getEntityId() that it will be returning either the entity ID for existing entities or that constant for new entities. Then we can safely use the constant as we've made a contract on the method what it will be returning.

2. +++ b/autosave_form.module
   @@ -144,3 +144,24 @@ function autosave_form_user_update(EntityInterface $user) {
   +  // We explicitly set entity_id to 0, because entity is not new and has id
   +  // already.
   

   But what if the method was overridden and wasn't returning 0 for new entities? We need to use the same logic everywhere.

Here I'm introducing a config parameter asked for in #26
Though I didn't understand this

check for that setting in AutosaveEntityFormHandlerInterface::isAutosaveEnabled()

Did you mean that this method should check just one config setting? Its name implies the opposite, that the whole check we make in hook_alter_form should be moved there. In both cases, I see a separate method as an overkill. To check for one config setting, it definitely is. To move all the logic defining if form should be altered - again, we use this logic just once, and check for the new setting separately in hook_entity_insert.

I have still rearranged code in hook_form_alter to make it more clear and eliminate duplicate actions or mere mismatches (eg, $entity_type_id variable was used to designate totally different things in different chunks of code - either the actual form entity type or the entity type expected by the router).

What bothers me is that we use hook_entity_update and hook_entity_insert which don't know about submitted form, was it autosave form or not and the entity of the form. And we try to use indirect logic to guess what entity was updated/created, and we have problems there. Cannot we move the logic from update and insert to form submit?

Then, about the route detection. Quite imaginary situation, but still, am I missing something or not: page which is routed to some entity add form, but which has blocks that contain other entity of the same type add forms. Will the detection work correctly?

And last: failing test actually should fail, because it makes sure there is no autosave js on create form. Well, with this update it should pass because I set the default of new config variable to false, but in general, we would need some test for this patch.

#34 worked well for me, thanks very much @gease @hchonov. Anything I can help with in this issue to push it forward?

#34 also working well for a site where we have deployed it. In our case with a pretty complex node type (many fields and lots of entity references, media, etc.). I'm not sure if @gease's comments in #34 hold anything back here, so marking RTBC for now.

The new function "autosave_form_entity_insert" generate a delete query for every entity save on the system.

It would be better to limit it to the entity select in the admin config form of autosave.
That would reduce a lot the not necessary delete query.

Reroll and a small fix.

Marking Needs Review. The updated patch in #39 at least adjusts the update hook name to prevent a collision with the latest version of the module.

I didn't do a full review of the code/functionality but just noticed this:

+++ b/src/Form/AutosaveEntityFormHandler.php
@@ -293,7 +293,7 @@ class AutosaveEntityFormHandler implements AutosaveEntityFormHandlerInterface, E
     list($form_id, $entity) = $this->getFormIDandEntity($form_state);
-    $allowed = !$entity->isNew() && !$this->currentUser->isAnonymous();
+    $allowed = !$this->currentUser->isAnonymous();

$entity isn't used anymore after this change, so the line above the changed one be dropped.

Code from #39 updated and tested. Ready for review.

For easier reviewing I created 2924006-44.patch from the merge request !8 and did an interdiff between #39 and #44.

Patch 39 working here for new items. (Have not fully reviewed source).

Tested patch in #45 and it applied cleanly and worked fine on the current version of the module (8.x-1.4). However, after I removed the patch, the "saving draft" notification keeps appearing right away whenever I edit a page. The current time interval I have setup to trigger autosave is 20000 milliseconds. So I shouldn't see the "saving draft" notification right when I edit a page. I doubled checked module code to make sure none of the patch code was still present and it wasn't. Not entirely sure if this is related to patch or not but I thought I should share.

This should fix the failing test as now it is expected that the library is loaded on new entity forms.

Tests are trying to use 'classy' which no longer exists in D10. Let's try with 'starterkit_theme'.