Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.2] Correct display of user profile #45107

Open
wants to merge 4 commits into
base: 5.2-dev
Choose a base branch
from

Conversation

AkameOuO
Copy link
Contributor

Pull Request for Issue #45079 .

Summary of Changes

It tries to check if keys are registered in HTMLHelper. But keys are not registered until the first call to HTMLHelper::(). Therefore, I call HTMLHelper::() without checking if keys are registered and use a catch block when the key is invalid.

And add two methods in Joomla\Component\Users\Administrator\Service\HTML\Users for colorScheme and allowTourAutoStart.

Testing Instructions

Reproduce the steps as described in the original issue.

Actual result BEFORE applying this Pull Request

Display raw values from database.
image

Expected result AFTER applying this Pull Request

Display sanitized texts.
image

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@fgsw
Copy link

fgsw commented Mar 11, 2025

I have tested this item ✅ successfully on 8287cc7

Before Pull Request

1-2

After Pull Request

2-2

Using - Use Default - show with and without PR the same result:

1-1


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45107.

@dautrich
Copy link

I have tested this item ✅ successfully on 8287cc7


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45107.

@alikon
Copy link
Contributor

alikon commented Mar 11, 2025

rtc


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45107.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Mar 11, 2025
try {
echo HTMLHelper::_('users.' . $key, $field->value);
break;
} catch (\InvalidArgumentException $e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HTMLHelper::_ throws InvalidArgumentException at a few places, so here you catch all of them, not only the one you want to catch. So you will hide other errors, too, with that code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better if I revert this part and manually register the function with HTMLHelper before it is called?

try {
echo HTMLHelper::_('users.' . $key, $field->value);
break;
} catch (\InvalidArgumentException $e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as my previous comment.

@richard67
Copy link
Member

Adding the "RMDQ" (release managers decision queue) label as I have some doubts if this fix is good as it is.

@richard67 richard67 added the RMDQ ReleaseManagerDecisionQueue label Mar 11, 2025
@bembelimen
Copy link
Contributor

Hello @AkameOuO
thanks for the PR. We discussed it in the Maintainer-Team and we agree, that we should add the two methods to the Users file. But we don't want to change the component tmpl files. So could you please revert that changes?
Also when checking I don't see the Atum-Parameter translated (still "10" for me)

@richard67
Copy link
Member

Back to pending due to requested changes. See previous comment.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/45107.

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Mar 12, 2025
@richard67 richard67 added Updates Requested Indicates that this pull request needs an update from the author and should not be tested. and removed RMDQ ReleaseManagerDecisionQueue labels Mar 12, 2025
@AkameOuO
Copy link
Contributor Author

AkameOuO commented Mar 13, 2025

@bembelimen

It still needs other patches for fixing the issue if reverting changes to tmpl files. I will try the method in this comment #45107 (comment).

before ec40c91:
image

after ec40c91:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug PR-5.2-dev Updates Requested Indicates that this pull request needs an update from the author and should not be tested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants