Creating Custom Block with same name as existing block throws SQL error

Adding to my list, feel free to reassign

Possibly related #2041795: Integrity constraint violation: 1062 Duplicate entry ... line 404 of Entity/DatabaseStorageControllerNG.php

This seems to be fixed in HEAD. Please re-open if you think it's still an issue.

berdir reported seeing this again.
will tackle this afternoon time permitting

Should be red/green

Follow up to remove procedural wrappers is here #2060865: Modernize custom_block.module forms

Doh! Of course you only validate the value if its a new block.
Also removed call to entity_load_multiple_by_properties in favour of EntityStorageControllerInterace::loadByProperties().

It is green. It is major. It has tests. RTBC.

+      $exists = \Drupal::entityManager()->getStorageController('custom_block')->loadByProperties(array('info' => $form_state['values']['info']));

Ouch. :( I actually find:

+    $exists = entity_load_multiple_by_properties('custom_block', array('info' => $form_state['values']['info']));

...much more readable. Is there a reason not to do it that way?

We shouldn't be using any of them really. It would be better if we injected $entityManager so we had:

$this->entityManager->getStorageController('custom_block')->loadByProperties(array('info' => $form_state['values']['info']));

I noticed we do have another issue here to clean up the procedural code. #2060865: Modernize custom_block.module forms so either way, it's going to change when that lands.

In general we don't want to use global function calls in classes at all if we can help it.

Yeeeeeahhhhh, that is not an improvement. :)

In the interest of making progress here, I've opened #2061887: It requires 4 levels of nested methods to figure out if a content entity exists as a follow-up issue, since the current situation is not this patch's fault.

At any rate, thanks for the bug fix and the test!

Committed and pushed to 8.x. Thanks!

Another reason

$this->storageController->loadByProperties()

is better is in the long run it lets us write UnitTests, eg see the test added here #1951268-93: Convert /forum and /forum/% to new router, remove forum_forum_load(), forum_get_topics(), create Forum service we would need a full-webtest and a use case for that code path if we used (for example) entity_load_multiple_by_properties() because we need a fully bootstrapped Drupal to access that function. This would take up to 2min locally to test (thats about the min for a Simpletest WebTestCase on my local). A phpunit test is done in < 15sec.
It takes getting used to, but the end result is cleaner in my opinion.
FWIW I'm not a trained CS student. I have no computing training, purely self taught from trial and error.
I started with mysql_connect (non Drupal), learnt Drupal and graduated to db_query(), then db_select() and PDO. The next step in my progression is $this->database->select(). So basically I'm getting a free CS education from the Drupal community ;).

Think that was a cross-post.

The reasons against procedural functions in classes make sense. I want to be clear that I'm fine with OO code. I'm not fine with so many levels of abstraction as to render a given line of code meaningless without the reader having an in-depth understanding of the entire stack of technology underpinning that led to it. But that's an argument for #2061887: It requires 4 levels of nested methods to figure out if a content entity exists. ;)

Thanks