correct me if I am wrong but this code essentially does the same thing right?

//code 1
param arguments.value = invoke( this, "get" & arguments.key );

//code 2
if(!isDefined('arguments.value')){
	arguments.value = invoke( this, "get" & arguments.key );
}

Excluding possible funny business with a variable called

variables.arguments.value

yeah, it seems the same.

I think there is a subtle difference in that cfparam will throw an error if invoke returns a null value

For the pull req, I think

structKeyExists

might be a bit more C21st than using

isDefined

.

isDefined

has not really been recommended in this sorta situation since CFMX6. Or - better, now that I look at the pull req and the surrounding code - to keep it analogous to the code following it, just

isNull

?

if (isNull(arguments.value) {
    arguments.value = invoke( this, "get" & arguments.key );
}

Also the comment you have in the code doesn't really belong there, because yer referring to code that doesn't exist any more (as you have removed it in the very pull req you are making), so it's lost its context. It'd be a good comment on the pull req itself, that said. I would probs wanna write some tests for this sort of low-level change too, given it's in

BaseEntity

. I dunno how strong this project is on testing though.

@Adam Cameron im quite familiar with the quick tests and I am confident that this change would be covered.

Well... obviously there's no case for when the getter returns null because otherwise the tests would be broken, right?

As far as the comment in the code goes it was more just a reminder in case someone would look at and think to themselves that it could be done with cfparam. And point taken on the tests, I was thinking the more high level test would cover it.

Nice one 😄