-
Notifications
You must be signed in to change notification settings - Fork 243
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
Fix EZP-23499: Fatal error removing class with unexistant datatype. #1094
Fix EZP-23499: Fatal error removing class with unexistant datatype. #1094
Conversation
It seems nice to me. If for some reason the developer has missing attributes, that's he's problem in the end. Imo, if he really wants to remove the class and leave some nonused content in the database, he should be able to. |
+0,8 Ideally there should be some script or something to remove attributes of a certain type. if all code needs to be paranoid about data returned from db it ends up being lots of output checks. |
+1 |
@@ -857,6 +857,14 @@ function removeAttributes( $removeAttributes = false, $version = false ) | |||
foreach ( $classAttributes as $classAttribute ) | |||
{ | |||
$dataType = $classAttribute->dataType(); | |||
if ( $dataType == null ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you're checking with "==" (type flexible), but above with "!==" (strict). I'd expect both to be strict, or both to be non-strict. Or, how about doing instanceof checks in both cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good, thanks.
I'll update to a strict check for simplicity, because this (actually, eZDataType::create()
) should always return either a datatype or null
.
(Even considering the possibility of loading an instance of some other class, a PHP fatal error would be appropriate in that case, I believe :) )
15f9a5e
to
0790582
Compare
@andrerom: I tend to agree, but wouldn't that be more of an improvement? |
yes it would be |
if ( $dataType === null ) | ||
{ | ||
eZDebug::writeError( | ||
"Skipping removal of class attribute for non-existant datatype " . $classAttribute->DataTypeString, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: s/non-existant/non-existent
+1 (when @lolautruche's concerns are closed) |
0790582
to
88875aa
Compare
Updated, thanks for the feedback. |
if ( $dataType === null ) | ||
{ | ||
eZDebug::writeError( | ||
"Skipping removal of class attribute for non-existant datatype " . $attribute->DataTypeString, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/non-existant/non-existent
+1, but you still have 2 typos :-) |
88875aa
to
cc73a1c
Compare
all done, thanks again. |
Fix EZP-23499: Fatal error removing class with unexistant datatype.
JIRA: https://jira.ez.no/browse/EZP-23499
This prevents a PHP fatal error when removing a class (contentType) if a datatype has been removed.
Instead, it is logged to error.log