Bugfix/strict param types master merged

[codecov-commenter]

Codecov Report

Merging #1849 (373d413) into master (94069d2) will decrease coverage by 14.75%. The diff coverage is 51.70%.

@@              Coverage Diff              @@
##             master    #1849       +/-   ##
=============================================
- Coverage     87.75%   73.00%   -14.76%     
- Complexity     7761     7786       +25     
=============================================
  Files           282      283        +1     
  Lines         21291    21261       -30     
=============================================
- Hits          18684    15521     -3163     
- Misses         2607     5740     +3133     

Flag	Coverage Δ
5-max	73.00% <51.70%> (-14.76%)	:arrow_down:
7.4	73.00% <51.70%> (-14.76%)	:arrow_down:
agnostic	?
mysql	68.77% <51.70%> (-0.25%)	:arrow_down:
pgsql	68.78% <51.70%> (-0.25%)	:arrow_down:
sqlite	66.63% <51.70%> (-0.24%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../Propel/Generator/Builder/Om/AbstractOMBuilder.php	93.25% <0.00%> (-3.64%)	:arrow_down:
src/Propel/Generator/Model/Behavior.php	84.70% <0.00%> (-9.05%)	:arrow_down:
src/Propel/Generator/Model/ScopedMappingModel.php	79.16% <0.00%> (-16.19%)	:arrow_down:
...c/Propel/Runtime/ActiveQuery/BaseModelCriteria.php	79.48% <0.00%> (-16.11%)	:arrow_down:
src/Propel/Runtime/ActiveQuery/Join.php	54.63% <0.00%> (-27.38%)	:arrow_down:
src/Propel/Runtime/Formatter/AbstractFormatter.php	76.19% <ø> (ø)
src/Propel/Generator/Model/Table.php	72.33% <21.05%> (-19.99%)	:arrow_down:
src/Propel/Runtime/Map/TableMap.php	79.56% <31.81%> (-19.28%)	:arrow_down:
...rc/Propel/Generator/Builder/Om/TableMapBuilder.php	95.27% <50.00%> (ø)
src/Propel/Runtime/Map/RelationMap.php	80.00% <50.00%> (-13.16%)	:arrow_down:
... and 163 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 94069d2...373d413. Read the comment docs.

[dmytro-dymarchuk]

It is very likely that I am just not in the loop and do not understand what you are doing here. So feel free to ignore my comments if they do not apply.

It looks like you are adding a lot of getXyzOrFail() methods to allow handling of null values at fields that simply should never be null, like the name of a table or a column. And now I am wondering if this is the right approach, or if those methods just paint over pretty serious problems, which is that objects can be in invalid states. If I managed to create broken table objects, I would want to know about the problem when it occurs, not somewhere down the line. And if those null values are only possible due to convenience during creation, I think we should fix that, instead of kicking the can down the road. With some of the objects you changed, it probably make sense to allow null values. But the places I checked (those I commented on), I am really not sure if it does.

Do you know what I mean? Let me know what you think

Hi. Unfortunately, it is hard to determine which property should not be nullable. This code doesn't hide problems since it throws exceptions. The main point of such methods is just allowing us to avoid null-checking on the client-side. Also, there is only one way to make properties not nullable and it is putting it into the constructor or setting a default value. But such changes might have a critical impact on client projects. We can't be sure how it is used.

[mringler]

Hi. Unfortunately, it is hard to determine which property should not be nullable. This code doesn't hide problems since it throws exceptions. The main point of such methods is just allowing us to avoid null-checking on the client-side. Also, there is only one way to make properties not nullable and it is putting it into the constructor or setting a default value. But such changes might have a critical impact on client projects. We can't be sure how it is used

Yeah, but that is what I mean with painting over the problem. This looks a lot like a workaround for smelly or not understood code inside Propel, and I am not sure if we do anyone a real favor by making it convenient to work around the error. Particularly since this introduces public functionality, which is hard to remove once the actual problem is solved. From what I see, most of the instances are not in client space, so I am not sure why users would interact with them at all. Things like Behavior, Join, RelationMap, even Column and Table are Propel internals, if people use them, they have to be aware that they can change. But again, I don't think they do, so I am not even sure who would call TableMap::getClassNameOrFail(), and it is well possible that I am just unaware, which is why I am asking, if you are sure that we need this. Do you have at least one use case in mind for each of the methods you added?

I do understand that you are trying to do something else, but in my experience, workarounds tend to make the issue worse, not better.

[dereuromark]

We should definitly look into methods than make no sense to return null. We have to find a good way to keep BC for the ones that can (DTOs) with introducing new null-safe ones. But I agree that we dont have to bloat the API on the cases where null just isnt justified.

Maybe we can outline each case in this PR and double-check? Then we should be able to move forward the next days.

[dereuromark]

@dmytro-dymarchuk There are open comments that need to be addressed and resolved.

[dereuromark]

@mringler Any blocking issues in your opinion? We could still clean up the API in the next days/weeks before release. If we can find certain methods to be not needed or that they can be improved in signature.

But we probably need to get going with also other tickets in the meantime to finalize the beta release preparation.

[mringler]

@mringler Any blocking issues in your opinion?

All good from my side. I've seen that @dmytro-dymarchuk changed some of the constructors, where null values would not make sense, so this is great,