search

kaliop-uk / ezmigrationbundle

This bundle makes it easy to handle eZPlatform / eZPublish5 content upgrades/migrations
GNU General Public License v2.0
53 stars 81 forks source link

"Could not find 'content' with identifier 'id: 14'" is not a satisfying error message. #215

Closed Rubinum closed 5 years ago

Rubinum commented 5 years ago

Hey,

I lost a day by figuring out following error message:

  Could not find 'content' with identifier 'id: 14'

during my kaliop migrations which changes passwords of certain users.

-
    type: 'user'
    mode: 'update'
    match:
        email: 'some@user.com'
    password: 'somePassword'

I am new to ez and after hours of debugging, I stumpled over following code line

https://github.com/kaliop-uk/ezmigrationbundle/blob/80de3b56c564b5c0b47b3bfc10a1121e5c5eec7c/Core/MigrationService.php#L519

Could you maybe enhance the error message, when the standard ez admin user is not found? I was confused by the error message "content not found", because my migrations only manipulate user stuff so why did the command load content and why is there a id: 14?

Not everyone knows, especially newbies, that there is a admin user account with the database ID 14 in the standard rollout of ez.

I've learned my lesson but it would be very nice, if we could enhance this error message :).

gggeek commented 5 years ago

I am sorry that you lost time needlessly for a cryptic error message. I will take a look at the best way to improve the situation.

However, let me point out a couple of things, to avoid misunderstandings and further disappointment:

  1. to be fair, the current README file does have a mention already about using 'admin' account with user ID 14

  2. indeed the fact that the default admin user id is 14 is a well-known fact to experienced ez developers

  3. as well as the fact that each user is attached to a content object. So object 14 === Admin user

  4. by its very nature (basically the flexible content model), eZ as a cms does sometimes emit error messages that are a bit generic and hard to debug without a lot of context and prior knowledge.

By default the migration bundle does not attempt to try to catch every possible error coming from the cms core to wrap it up in an error message that would make it more explicit by adding the 'context info' that says which migration operation was running that triggered the fault in the kernel.

Going by memory, an example: if you pass unexpected values for a match condition, you will not only get no elements matched, but most likely an hard-to-understand error, too - but at least by default matchers are set up to error out on zero matches, to avoid silent mistakes...

It would be a huge undertaking to foresee every possible error case, considering that the whole migration bundle is basically little more than a wrapper around the existing api of the cms. We would in the end have to validate every parameter coming from user input, and then change the validation rules whenever the kernel itself changes its inner workings (eg. are non-integer ids allowed or not?). Too much work for the current maintainers team...

Using a debugger such as xdebug which allows one to halt on exceptions and dumping the exception chain is one way to help troubleshooting. Even adding a var_dump(debug_backtrace()); die(); at the point where an exception is thrown can be a simple but useful debugging technique.

Wrapping it up: I will do my best, but expect no miracles ;-)

Rubinum commented 5 years ago

I am happy to hear, that you will take a look. :) My issue was solved by using xdebug at the end, so I followed your advice intuitively 😄.

I fully understand, that providing a better error message for every possible input, would lead into a lot of work and it would compensate some weaknesses of the framework more likely. Thats clearly not the mission for this library 😄.

Better error message in general would be nice, but maybe we don't have to change the code for a general solution. Maybe it would be ok, if we provide better error messages for errors, where we know the concrete context (e.g. this issue, where we know, that the standard admin user is missing). This would lower the scope a bit. There could be more errors, where we know the context and could provide a better error message.

gggeek commented 5 years ago

Fixed in master.

The exception thrown in this scenario will now be of class InvalidUserAccountException. The error message will read: Could not find the required user account to be used for logging in: '14'. UserService says: Could not find 'content' with identifier 'id: 14'.

Would that be ok ?

gggeek commented 5 years ago

ps: added tests for the new exception, all green now

gggeek commented 5 years ago

Implemented in 5.10