Comments

larowlan’s picture

Status: Active » Needs review
StatusFileSize
new3.16 KB
new2.7 KB

Patch with test demonstrating fail and then fix.

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

jhodgdon’s picture

Component: documentation » database system

Not a documentation issue.

larowlan’s picture

Component: database system » entity system

sorry @jhodgdon

webchick’s picture

Component: entity system » database system
Status: Reviewed & tested by the community » Needs work

This is somewhat minor, but AFAIK the standard is not to use Exception but instead to specify \Exception where it is used.

webchick’s picture

Component: database system » entity system

Cross-post-o-rama!

larowlan’s picture

@webchick, http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/lib/D... has 'use Exception', I'm not fussy either way - can change both if you like.

webchick’s picture

Yeah, that's wrong. The reason for that is because if you forget the "use Exception" thing at the top, then basically stuff doesn't work. Since it's not easy to grep for ensuring for every "Exception" there's a corresponding "use Exception", we introduced a standard to prefix it with "\" everywhere so offenders are easier to pick out. This is true for any internal PHP class: See http://drupal.org/node/1353118 under "use"ing classes.

larowlan’s picture

Status: Needs work » Needs review
StatusFileSize
new4.05 KB

thanks, again in awe of how you keep all this in your head, new patch fixed both files.
sorry, no interdiff

Status: Needs review » Needs work

The last submitted patch, database-storage-controller-ng-1885542.9.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
larowlan’s picture

Not sure what happened there, fresh re-roll against HEAD.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Looks sane enough.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, database-storage-controller-ng-1885542.12.patch, failed testing.

tim.plunkett’s picture

I'm missing something. Hmmm

larowlan’s picture

Status: Needs work » Needs review
StatusFileSize
new4.39 KB
new607 bytes

meh, missed one
find and replace fail.

webchick’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. Marking back to RTBC so it can be committed once tesbot's done verifying.

sun’s picture

Status: Reviewed & tested by the community » Needs review

Good catch.

Can we have a test-only patch to prove the test?

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new4.39 KB
new2.69 KB

Wait for red/green.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Great, thanks!

Committed and pushed to 8.x.

Status: Fixed » Closed (fixed)

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