Umbraco-CMS icon indicating copy to clipboard operation
Umbraco-CMS copied to clipboard

Fixes: Consistency for Nullable reference types in LINQ extensions methods #12692

Open DanDiplo opened this issue 3 years ago • 1 comments

Prerequisites

  • [x] I have added steps to test this contribution in the description below

If there's an existing issue for this PR then this fixes - https://github.com/umbraco/Umbraco-CMS/issues/12692

Description

Removes some of the pain around nullable reference types.

Makes querying Children() and similar axis methods simpler by ensuring they never return null - they will always return an empty collection.

Also ensure that the Root() method can never return null, as there always should be root content.

Finally, ensures the .Name property can never be null. It will default to string.Empty. Again, this makes referencing it simpler as avoids the need to add extra null checks.

All Unit tests pass.

DanDiplo avatar Jul 15 '22 19:07 DanDiplo

Hi there @DanDiplo, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

  • It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
  • The automated tests all pass (see "Checks" tab on this PR)
  • The level of security for this contribution is the same or improved
  • The level of performance for this contribution is the same or improved
  • Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
  • If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
  • [x] 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

github-actions[bot] avatar Jul 15 '22 19:07 github-actions[bot]

Sorry for the delay @DanDiplo - we rebased your changes and this all seems good to go for v11. Thanks again for the help! 👍

nul800sebastiaan avatar Oct 19 '22 09:10 nul800sebastiaan

@nul800sebastiaan No worries, glad to help! Looking forward to v11

DanDiplo avatar Oct 19 '22 10:10 DanDiplo

Hi,

looks like this makes a breaking change?

(specifically, https://github.com/umbraco/Umbraco-CMS/pull/12703/files#diff-04ea84454360ff165d2411fe7f2fb64ceb3f8f2ee5de57789a0f424d35608a5fL694-L699 )

For example on the Umbraco starter kit - if you add the following to the master.cshtml template:

@{
    var productsPage = Model.AncestorOrSelf("Products");
    if (productsPage != null) {
      <h2>[@productsPage.Name]</h2>
    } else { 
      <h3>No Product parent</h3>
      }
}

in v10 or below you would expect "No Product parent" on any page that isn't the poducts or a product page. so for example on the homepage it would say "No Product Page"

on v11.1 this returns "[Home]" when on the homepage. which means you can no longer use the logic above if you want to put something in a master or parent template that only renders in certain parts of your site tree.

KevinJump avatar Feb 02 '23 11:02 KevinJump

@KevinJump I think you are right in regard to the AncestorOrSelf method. The idea was that all LINQ methods that returned collections should never return null - they should always return an empty collection - so you could chain methods without worry. But the AncestorOrSelf() method looks like it should return null if not found as it just returns a single node. Not sure why I changed that...

I think the other methods are OK, though? Is it just that one? I can do a PR to fix if it's just that...

DanDiplo avatar Feb 03 '23 09:02 DanDiplo

I ran in to this issue when upgrading a v9 site to v11. @DanDiplo you are correct that it should return null. Can you make a pull request to fix it as you suggested?

ixitab avatar Mar 09 '23 08:03 ixitab