I have tested this item ✅ successfully on 91a699e

Marking as a success because it fixes the problem but I have no idea what the code was supposed to have done or why it was added

I have tested this item ✅ successfully on 91a699e

Marking as a success because it fixes the problem but I have no idea what the code was supposed to have done or why it was added

Marking as a success because it fixes the problem but I have no idea what the code was supposed to have done or why it was added

Was added due to suggestion by @wilsonge : #25451 (comment).

@SharkyKZ Title and testing instructions of this PR say it fixes only the contacts view, but in addition to that it fixes the articles view (administrator/components/com_content/Service/HTML/AdministratorService.php), or am I wrong?

If it's required it's required in both files.

But I don't get this. Before the PR this was a required variable - this was the signature.

public function featured($value = 0, $i, $canChange = true)

As far as I can see this is always used with a value. So how is this ever taking the default value of the function of 0. Or are we saying in some case here https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/components/com_contact/tmpl/contacts/default.php#L127 $i is somehow 0?

I'm not disputing we somehow broke this btw. But to understand the correct fix I need to understand why it's broken. Reverting that part isn't good enough - because if 0 is a valid value then it's a really bad default (I really wish we didn't have to set a default but.....)

Or are we saying in some case here https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/components/com_contact/tmpl/contacts/default.php#L127 $i is somehow 0?

Yes, $i is the array key:

@wilsonge If $i is an array key, would it make sense then to use -1 as default value and raise the exception if lower than zero?

Or maybe just make the parameter mandatory? No need for exceptions then.

I have just come across this problem.

Does anyone know why $i was given a default value?

In the case of $published, the parameter $i is a mandatory parameter.

In my opinion, we should keep this consistent in the project.

It's because of codestyle:

Arguments with default values must be at the end of the argument list

In the case of Joomla\Component\Contact\Administrator\Service\HTML::featured($value = 0, $i, $canChange = true) mandatory parameter $i was after optional parameter $value.

Maybe we could make $value required instead.

@SharkyKZ Thanks for explaining.

I would suggest that the value parameter becomes a mandatory field. In my opinion, it is a mistake if value is not set. This error would then have to throw an error and be properly processed further.

Required $value parameter would also be more consistent to astridx's code snippet. Reading the code of these 2 functions modified by this PR, I think the $value parameter should be required, then you also can make the $i required. The exceptions you could add back and change them so $i lower than zero are not allowed, if that is useful.

When testing this we should test both contacts view and com_content stuff, e.g. articles view, because this PR modifies files for both.

OK let's make $value required. I think in this case it's acceptable

Thanks!