See #1513210: Meta: Start converting module provided classes to PSR-0

core/includes/update.inc:class DrupalUpdateException extends Exception { }

CommentFileSizeAuthor
#20 update-exception-1545068-20.patch3.05 KBberdir
#17 update-exception-1545068-16.patch3.04 KBberdir
#12 update-exception-1545068-12.patch3.12 KBberdir
#10 systemupdate-it-is-1545068-10.patch3.1 KBberdir
#4 update-exception-1545068-4.patch3.09 KBberdir
#2 update-exception-1545068.patch970 bytesberdir

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue tags: +PSR-0
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new update-exception-1545068.patch970 bytes

Simplest PSR-0 patch evar ;)

Wasn't sure about the namespace, though. We already have Updater, so I thought that maybe Drupal\DatabaseUpdate is clearer and also maps to the component here on d.o.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new update-exception-1545068-4.patch3.09 KB

Ok, would help if I would convert the usages of that exception too.

We don't use it actually, only in documentation blocks and hook_update_N() documentation ;)

Status: Needs review » Needs work
Issue tags: -PSR-0

The last submitted patch, update-exception-1545068-4.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
Issue tags: +PSR-0

#4: update-exception-1545068-4.patch queued for re-testing.

Crell’s picture

Crell commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/DatabaseUpdate/UpdateException.php
@@ -0,0 +1,13 @@
+namespace Drupal\DatabaseUpdate;

This should be Drupal\Core\DatabaseUpdate.

Strictly speaking, the update system isn't just database updates. People run other changes all the time, to reformat configuration. I suspect in D8 that will be more common, with configuration living in CMI files, it will still need to be cleaned up. (Deleted a variable, renamed a variable, etc.) So perhaps Update is the correct subsystem name? Or SystemUpdate?

+++ b/core/lib/Drupal/Core/DatabaseUpdate/UpdateException.php
@@ -0,0 +1,13 @@
+class UpdateException extends Exception { }

A number of the other exceptions we've been converting to extend from RuntimeException to be a bit more accurate. We may as well do that here. It will need a "use" statement.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

wow, "Updater" really is a poor component name to begin with... that should be renamed to something more self-descriptive. Separate issue.

I think a proper component name here would be "SystemUpdate". (Update on its own could mean everything and the world.)

(oh, didn't see that @Crell suggested the same name! Nice! :))

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yeah, I did initially go for Update as well. The problem is that we then have Updater and Update namespaces, which are something *completely* different. So I did go for DatabaseUpdate because that matches with with the component name "database update system". Which has been renamed from update system, I guess to avoid being mixed up with the update module.

Edit: SystemUpdate would be fine with me as well.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new systemupdate-it-is-1545068-10.patch3.1 KB

So be it.

- Changed to SystemUpdate
- extends RuntimeException
- Added use for that. As I said, the class is not actually used, otherwise this would have already failed with the Exception as well.

Crell’s picture

Crell commented
Status: Needs review » Needs work

When Crell and sun independently agree on something, you know there's something to it. :-)

+++ b/core/lib/Drupal/Core/SystemUpdate/UpdateException.php
@@ -0,0 +1,15 @@
+/*
+ * @file
+ * Definition of Drupal\DatabsaeUpdate\UpdateException.
+ */

Didn't update this. And there's a spelling error. And it's missing \Core.

+++ b/core/lib/Drupal/Core/SystemUpdate/UpdateException.php
@@ -0,0 +1,15 @@
+namespace Drupal\SystemUpdate;

Still missing \Core.

+++ b/core/modules/system/system.api.php
@@ -5,6 +5,8 @@
+use Drupal\SystemUpdate\UpdateException;

Missing \Core.

I think we definitely need to add a test here to catch this, even if it's a trivial unit test.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new update-exception-1545068-12.patch3.12 KB

Hah, serious fail.

This should be better.

As for the tests, we could add an update hook implementation to a test module and throw an exception there. Right now, there is nothing that could be tested other than manually throwing an exception directly in the test code.

Crell’s picture

Crell commented

That seems like a reasonable test at first blush. Or #12 looks good now on its own on visual inspection.

robloach’s picture

robloach
he/him
commented

#12: update-exception-1545068-12.patch queued for re-testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Not sure if the tests should be a follow-up. I don't think we have any tests yet that actually test what happens when an update fails, that could be quite a task.

robloach’s picture

robloach
he/him
commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/DatabaseUpdate/UpdateException.phpundefined
@@ -0,0 +1,13 @@
+
+namespace Drupal\Core\DatabaseUpdate;
+

Do we really need a whole new Drupal\Core\DatabaseUpdate namespace for this? Couldn't we just put this in Drupal\Core\Utility\UpdateException or something?

27 days to next Drupal core point release.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new update-exception-1545068-16.patch3.04 KB

Fine with me, we can still move it back once there is more there than just an exception class...

robloach’s picture

robloach
he/him
commented
Status: Needs review » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Sorry folks needs a quick re-roll for this typo, which appears twice:

+ * Drupal\Core\Utlity\UpdateException with an appropriate error message, for

Utlity.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new update-exception-1545068-20.patch3.05 KB

All were wrong, actually. Fixed.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x.

Automatically closed -- issue fixed for 2 weeks with no activity.