Skip to content

DOCSP-35951: write operations delete docs #2829

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

Conversation

ccho-mongodb
Copy link
Contributor

@ccho-mongodb ccho-mongodb commented Apr 8, 2024

JIRA:
https://jira.mongodb.org/browse/DOCSP-35951

Staging:
https://preview-mongodbcchomongodb.gatsbyjs.io/laravel/DOCSP-35951-write-operations-delete/fundamentals/write-operations/#delete-documents

Note:
This PR is stacked with all the write operations PRs and is the final section.

Relevant changes:

Checklist

  • Add tests and ensure they pass
  • Add an entry to the CHANGELOG.md file
  • Update documentation for new features

Copy link
Contributor

@rustagir rustagir left a comment

Choose a reason for hiding this comment

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

a few small things!

Comment on lines 504 to 505
- :ref:`Pruning <laravel-model-pruning>`, which lets you define conditions
in which a document should be deleted automatically
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- :ref:`Pruning <laravel-model-pruning>`, which lets you define conditions
in which a document should be deleted automatically
- :ref:`Pruning <laravel-model-pruning>`, which lets you define conditions
that mark documents for automatic deletion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing out the unclear wording. Since the conditions don't mark documents directly, so maybe I'll use "define conditions that qualify a document for automatic deletion".

Comment on lines 514 to 515
- Call the ``destroy()`` method on the model to delete a document by the
primary key.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Call the ``destroy()`` method on the model to delete a document by the
primary key.
- Call the ``destroy()`` method on the model to delete a document by matching the
primary key.

Copy link
Contributor Author

@ccho-mongodb ccho-mongodb Apr 9, 2024

Choose a reason for hiding this comment

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

I think the existing sentence and suggestion might be omitting a detail necessary to understand what to pass it, so I'll figure out how to reword. Will also update the description in the corresponding section for deleting multiple docs.

Comment on lines 535 to 536
The following example shows how to delete a document by calling ``delete()``
on an instance of the model:
Copy link
Contributor

Choose a reason for hiding this comment

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

S: separate the three examples into subsections or move into bullet points, but as of now stacking them is a bit confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with this and had considered earlier on, but wanted to keep all examples at the same heading level. Both Insert and Update use the same formatting with the same intention.

I think once these sections (Insert, Modify, Delete) are split into individual pages similar to drivers docs are separate pages, these can be their own sections.

In the meantime, I'll add literalinclude captions (including examples in the prior sections) to identify which method is being demonstrated.

Delete Multiple Documents Examples
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

You can perform deletes on multiple documents in the following ways:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
You can perform deletes on multiple documents in the following ways:
You can delete multiple documents in the following ways:

@ccho-mongodb ccho-mongodb requested a review from rustagir April 9, 2024 18:28
Copy link
Contributor

@rustagir rustagir left a comment

Choose a reason for hiding this comment

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

lgtm with a few last comments!

Comment on lines 512 to 513
- Call the ``destroy()`` method on the model and pass it a primary key to
delete the matching document.
Copy link
Contributor

Choose a reason for hiding this comment

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

S: revise to avoid the phrasing "pass it"
Ie

Suggested change
- Call the ``destroy()`` method on the model and pass it a primary key to
delete the matching document.
- Call the ``destroy()`` method on the model while passing a primary key to
delete the matching document.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed on Slack. Replacing with a adverbial phrase to describe the first part of the sentence.

@ccho-mongodb ccho-mongodb marked this pull request as ready for review April 10, 2024 16:15
@ccho-mongodb ccho-mongodb requested a review from a team as a code owner April 10, 2024 16:15
@ccho-mongodb ccho-mongodb requested review from jmikola and removed request for a team April 10, 2024 16:15
@GromNaN GromNaN requested review from GromNaN and removed request for jmikola April 11, 2024 14:55
Comment on lines 512 to 516
- Call the ``destroy()`` method on the model, passing it the primary key of
the document to be deleted.
- Call the ``delete()`` method on an instance of the model.
- Chain methods to retrieve and delete an instance of a model by calling the
``delete()`` method.
Copy link
Member

Choose a reason for hiding this comment

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

$model->delete() should be the first in the list as it's probably the more used.

The alternate Model::destroy($ids) accepts a single id, a list of ids, or a list of model intances.

Suggested change
- Call the ``destroy()`` method on the model, passing it the primary key of
the document to be deleted.
- Call the ``delete()`` method on an instance of the model.
- Chain methods to retrieve and delete an instance of a model by calling the
``delete()`` method.
- Call the ``$model->delete()`` method on an instance of the model.
- Call the ``Model::destroy($ids)`` method on the model class, passing it the id of
the document to be deleted.
- Chain methods to retrieve and delete an instance of a model by calling the
``delete()`` method.

Copy link
Contributor Author

@ccho-mongodb ccho-mongodb Apr 12, 2024

Choose a reason for hiding this comment

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

Question:
Is the "$model->delete()" a common way to represent a placeholder for an Eloquent model for a Laravel developer audience?

Otherwise, what would be a common way to explain calling on a method on an instance of a model? I could be wrong, but I suspect the current suggestion might be repetitive (the instance and method are shown, but it's still described as a method).

Copy link
Member

Choose a reason for hiding this comment

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

I believe that code is more explicit than words. This is the convention I use systematically on this project when I want to emphasize the scope of a method.

I've found examples of Model::method in Eloquent doc, but not $model->method. See https://laravel.com/docs/11.x/eloquent#deleting-models

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with changing conventions, but is this something we could come back to and decide later? I think it might involve updating several of the currently published docs to make them use it consistently. If so, I can ticket that.


// begin model delete one fluent
Concert::where('venue', 'Carnegie Hall')
->orderBy('_id')
Copy link
Member

Choose a reason for hiding this comment

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

I don't get the point of ordering for deleting. In fact, it's ignored:

public function delete($id = null)
{
// If an ID is passed to the method, we will set the where clause to check
// the ID to allow developers to simply and quickly remove a single row
// from their database without manually specifying the where clauses.
if ($id !== null) {
$this->where('_id', '=', $id);
}
$wheres = $this->compileWheres();
$options = $this->inheritConnectionOptions();
if (is_int($this->limit)) {
if ($this->limit !== 1) {
throw new LogicException(sprintf('Delete limit can be 1 or null (unlimited). Got %d', $this->limit));
}
$result = $this->collection->deleteOne($wheres, $options);
} else {
$result = $this->collection->deleteMany($wheres, $options);
}
if ($result->isAcknowledged()) {
return $result->getDeletedCount();
}
return 0;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, so if the where() query filter matched multiple documents, wouldn't it be important to specify the sort order such that it's known which one will surface when calling limit(1), since that's the one that will be deleted? Does that mean that you can't use indexes with sort orders for the query filter part of the delete operation?

If not possible, I should probably include that information.

Copy link
Member

Choose a reason for hiding this comment

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

That's currently not possible. That could be a new feature PHPORM-169

Copy link
Member

Choose a reason for hiding this comment

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

Does that mean that you can't use indexes with sort orders for the query filter part of the delete operation?

Note: from the server's perspective, there is no way to specify an order with a delete operation.

Indexes can certainly be used (and hinted). In the case of a sparse index that might actually have an impact on which document(s) get selected for the deletion.

@ccho-mongodb ccho-mongodb requested a review from GromNaN April 12, 2024 04:36
@ccho-mongodb ccho-mongodb requested a review from GromNaN April 12, 2024 13:47

In this section, you can learn how to delete documents from a MongoDB collection
by using {+odm-short+}. Use delete operations to remove data from your MongoDB
database permanently.
Copy link
Member

Choose a reason for hiding this comment

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

"permanently" is wrong when the SoftDeletes trait is used. But you mention it just below, so maybe that's okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for providing that perspective, I'll update the text to make that more clear!

@ccho-mongodb ccho-mongodb merged commit 95a08ce into mongodb:4.1-base-docs-write-operations Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants