Add return documentation for Merge::execute()

Code not being visible - That's a known/fixed bug in the API module (the fix just hasn't been deployed to api.drupal.org yet, but should be in a few days).

Lack of return value - agreed, should be fixed. Thanks for reporting!

Here's a new patch where we've added documentation for the execute method found in the file: core/lib/Drupal/Core/Merge.php .

Thanks to Josimar, Julio and David.

Thanks for the patch! There are a few problems:

a) The exceptions -- it should specifically document which exceptions it can throw, and under what circumstances, rather than a generic "@throws Exception".

b)

+   * Only if the queryOptions['throw_exception'] is TRUE (default), an exception
+   * could be thrown.

Can this be turned around to be clearer? Maybe something like "If there is a problem with the query and $this->queryOptions['throw_exception'] is TRUE (default), an exception will be thrown. Otherwise, a NULL value will be returned.

c) The @return section has a few problems:

+   * @return
+   *   - Marge::STATUS_INSERT: If the entry does NOT exists in the database,
+   *     it executes an INSERT.
+   *   - Merge::STATUS_UPDATE: If the entry exists then it executes an UPDATE.
+   *   - NULL: If there is an exception and queryOptions['throw_exception'] is
+   *   FALSE

- First line: Marge -> Merge
- First line: exists -> exist
- There should be a line introducing the list. Something like "One of the following values:"
- Last line needs to be indented two spaces and end with .
- We shouldn't have NOT be all-caps.
- Can we make the first two items more parallel in their grammatical structure? How about "If the entry already exists, and an UPDATE query is executed" and "If the entry does not already exist, and an INSERT query is executed"?

Thanks!

Added new patch.

Thanks for the new patch!

There are a few things to clean up:

a)

+   * If $this->queryOptions['throw_exception'] is TRUE (default)
+   * and there is a problem with the query, an exception will be thrown.
+   * Otherwise, a NULL value will be returned.

Can this be wrapped into one paragraph, closer to 80-character lines? Also, I think the last line "Otherwise..." is a bit ambiguous, because the if() in the first sentence is compound. Can it be more explicit like "If ... is FALSE and there is a problem with the query, a NULL value will be returned." rather than just saying "Otherwise"?

b)

+   *   One of the following values:
+   *     - Merge::STATUS_INSERT: If the entry does not already exist,
+   *       and an INSERT query is executed.
+   *     - Merge::STATUS_UPDATE: If the entry already exists,
+   *       and an UPDATE query is executed.
+   *     - NULL: If there is an exception and queryOptions['throw_exception'] is
+   *       FALSE.

This list is not formatted correctly. See https://drupal.org/coding-standards/docs#lists

I also don't like the last item -- "if there is an exception" when above you said "if there is a problem".

c)

+   * @throws \Drupal\Core\Database\Query\InvalidMergeQueryException
+   * @throws \Drupal\Core\Database\IntegrityConstraintViolationException
+   * @throws \Exception

Not having a description on the first two exceptions is probably fine, because they are self-documenting. But for the last one, you need to describe when the generic "Exception" could be thrown. See
https://drupal.org/coding-standards/docs#throws

This is still valid, we should start with addressing #5.

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Database%...

Not able to apply patch given in #4, reroll needed.

Feedback from #5 addressed.

Fixed PHPCBF issue.

1. +++ b/core/lib/Drupal/Core/Database/Query/Merge.php
   @@ -351,6 +351,28 @@ public function key($field, $value = NULL) {
   +   * If $this->queryOptions['throw_exception'] is TRUE (default)
   +   * and there is a problem with the query, an exception will be thrown.
   +   * If $this->queryOptions['throw_exception'] is FALSE and
   +   * there is a problem with the query, a NULL value will be returned.
   +   *
   

   The 'throw_exception' option is deprecated now, so I think we can just remove this piece.

2. +++ b/core/lib/Drupal/Core/Database/Query/Merge.php
   @@ -351,6 +351,28 @@ public function key($field, $value = NULL) {
   +   * @throws \Exception
   +   *   When the generic "Exception" could be thrown.
   

   I do not think we would ever throw a generic \Exception nowadays.

3. +++ b/core/lib/Drupal/Core/Database/Query/Merge.php
   @@ -351,6 +351,28 @@ public function key($field, $value = NULL) {
   +   * @throws \Drupal\Core\Database\Query\InvalidMergeQueryException
   +   * @throws \Drupal\Core\Database\IntegrityConstraintViolationException
   

   We can take from the comments in code and the exception messages and add here the occasion when the exceptions are thrown.

Feedback addressed mentioned in #19.
Please review.

Fixing code standard.

The docblock looks good to me.
All the points of @mondrake are addressed.
For me it is RTBC.

Sorry to put it back, I read the actual code and have a couple more points.

1. +++ b/core/lib/Drupal/Core/Database/Query/Merge.php
   @@ -351,6 +351,23 @@ public function key($field, $value = NULL) {
   +   *   - NULL: If there is a problem and queryOptions['throw_exception'] is
   +   *     FALSE.
   

   Since queryOptions['throw_exception'] is deprecated, I think it'd be better to reflect in the comment:

      *   - NULL: (deprecated) If there is a problem and queryOptions['throw_exception'] is  FALSE.
   

2. +++ b/core/lib/Drupal/Core/Database/Query/Merge.php
   @@ -351,6 +351,23 @@ public function key($field, $value = NULL) {
   +   * @throws \Drupal\Core\Database\IntegrityConstraintViolationException
   +   *   When trying to insert a row that would violate a unique key constraint.
   

   Actually I think this is never thrown outside the method. This exception is used within the method to catch INSERT failures and in that case resolve to UPDATE instead.

Created a MR based on the feedback in #23, but I'm not exactly sure in what to do about @throws \Drupal\Core\Database\IntegrityConstraintViolationException, moving to NR for feedback.

#26 just remove it. The purpose of listing the @throws in the docblock is to let developers know what exceptions they could expect to have to deal with. If its use is within the method, then this is not relevant.

Thanks @mondrake!

Removed, please review!

Looks good to me now, thanks!

Committed/pushed to 9.3.x and cherry-picked to 9.2.x, thanks!