User tests: Successful: Unsuccessful:
…splaying blank value
Pull Request for Issue #31355 // added by JM
Status | New | ⇒ | Pending |
Category | ⇒ | Administration com_banners |
This removes HTML filtering.
I do not see the problem described. And the test by @adj9 does not show the Clients screen. This is my screen grab without the patch applied:
I have not looked at the trim filter but I would expect that to remove leading and trailing white space only.
@ceford to replicate the issue you need to only put spaces in the field
I started with just a space in "John Doe" and that name appears in the Contact column without the patch. The screen grab by @adj9 looks like the Banners list but even there I see the client name. It is not empty. So for me the issue described does not exist.
ONLY put spaces and see what happens
Then try to create an article with a title containing ONLY spaces
@MeghaBiranje Is this pull request made for solving your issue #31355 ? If yes: Why haven't you referenced that issue by using the "Pull Request for Issue # ." at the top of the description? It just need to change it to "Pull Request for Issue #31355 ." Or at least leave in the issue a comment referring to this PR, or whatever else way to show the PR belongs to that issue. You can't expect maintainers to do a forensic research to find out which of YOUR pull requests solves which of YOUR issues.
I have deleted my Pass test. With the patch applied I can include html tags, such as <h1>, that are evident in the list view. That is an undesirable side effect.
@ceford It was not enough to delete the test comment. You should have used again the "Test this" button and then select the "I have not texted" (or so) test result. I've done that for you now, I've reset your test result so it isn't counted by the tracker and in the test on GitHub.
@richard67 thank you for the tips - it seems I am learning the hard way.
Related: is there a case for trimming string input by default?
I was looking for documentation on TRIM and concluded that all of the tutorials I could find are a bit dated. The code in InputFilter.php seemed the best place to look.
Looks like the problem here is that the ClientTable does not have its own check()
method where we could check that the text entered is empty. Therefore something like this:
/**
* Overloaded check function
*
* @return boolean True if the object is ok
*
* @see Table::check()
* @since {
*/
public function check()
{
try
{
parent::check();
}
catch (\Exception $e)
{
$this->setError($e->getMessage());
return false;
}
// Check for valid contact
if (trim($this->contact) === '')
{
$this->setError(Text::_('COM_BANNERS_PROVIDE_VALID_CONTACT'));
return false;
}
return true;
}
Labels |
Added:
?
|
Category | Administration com_banners | ⇒ | Administration com_banners Language & Strings |
Needs
use Joomla\CMS\Language\Text;
on top of file
Labels |
Added:
?
|
I have tested this item
I have tested this item
Status | Pending | ⇒ | Ready to Commit |
RTC
@infograf768 Could you approve the code on GitHub so the "Changes requested" disappears at the bottom of the PR which currently still is shown due to your previews review with change requests?
Labels |
Added:
?
|
Applied @Quy suggestion in #31354 (comment) .
No change for tests. Keeping RTC.
Status | Ready to Commit | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2020-11-22 08:52:15 |
Closed_By | ⇒ | infograf768 |
tks
I have tested this item✅ successfully on f94cf99

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