Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Notice error due undefined property + fix in mail address checker #1530

Closed
wants to merge 5 commits into from

4 participants

@Kubik-Rubik
Owner

Notice error due undefined property + removed checkstyle error

Domain must have at least one country code (except it is localhost)

@pasamio

Won't this cause a strict notice when the if fails because it triggers an implicit creation of an object?

@Kubik-Rubik
Owner

No, the object is $app, $app->registeredurlparams is a property of the object. The problem is that the extensions should set this property but not all extensions do it. The registeredurlparams object is created automatically in the first assignment $registeredurlparams->format = 'WORD';

Maybe we should create this object manually ofter the application call:

$app = JFactory::getApplication();

// Create empty object
$registeredurlparams = new stdClass;

@pasamio

I know the $registeredurlparams object will be created automatically but that will trigger a "Strict Standards: Creating default object from empty value" warning. If the point is to remove a notice then putting a new one in seems counterproductive.

@Kubik-Rubik
Owner

Sam, you were right. Now all warnings are removed!

@pasamio

You've now introduced a PHPCS problem with white space here, can you clean that up?

@Kubik-Rubik
Owner

Hi Sam, I have fixed the whitespace but the pull request checker still shows me the same error. Is it in the cache? Could you please take a quick look? Thank you!

Kubik-Rubik Domain must have at least one country code
Domain must have at least one country code (except it is localhost)
87e392e
@Kubik-Rubik
Owner

Wanted to do a new pull request, but the new changes were combined in this pull request (sorry, I'm new to GitHub).

The second request fixes a problem with the helper class for the mail address check. Domains must have at least one country code (except it is localhost) -> added this check to the helper.php.

@elkuku

You also have to make your pull requests against the staging branch.

@pasamio

Domains don't need at least one country code, the reality is that you can send mail with just a hostname in a properly configured system. In fact I've been annoyed in the past by bad assumptions around what a real domain constitutes, particularly when email addresses in the .local domain are rejected since they are valid on my network.

Why this has been put together is because you've made both commits on the same branch. You need to essentially create one branch per pull request. GitHub will automatically pick up new commits to the branch and include it in the pull request. Untangling branches can become complicated very quickly though.

@garyamort

Well, technically the address doesn't have to contain a domain at all. It is perfectly legal run :
echo “This will go into the body of the mail.” | mail -s “Hello world” root

Since the class has already acknowledged that it's rules are not RFC compliant, I don't see any reason not to make them more "reasonable" for the general use case... I just wish there was some way to easily override/change these rules without replacing the class.

@pasamio

@Kubik-Rubik Can you do an interactive rebase and remove the email domain commit and fix up/squash the other commits into one. You'll need to force push a change back to your repository to get this pull request updated. Then we can get this included.

You can read about squashing commits here:
http://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits

@pasamio

@Kubik-Rubik When you update this to remove the email domain commit and update the branch, please comment back here and we'll re-open the pull request. I'm going to close the pull request for now but that doesn't mean it's rejected but because you need to make some more changes before we can progress.

@pasamio pasamio closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 14, 2012
  1. Removed checkstyle error

    Kubik-Rubik authored
Commits on Sep 16, 2012
  1. Removed Strict Standard warning

    Kubik-Rubik authored
Commits on Sep 17, 2012
  1. Fixed "Whitespace found at end of line"

    Kubik-Rubik authored
Commits on Sep 19, 2012
  1. Domain must have at least one country code

    Kubik-Rubik authored
    Domain must have at least one country code (except it is localhost)
This page is out of date. Refresh to see the latest.
View
7 libraries/joomla/cache/cache.php
@@ -654,8 +654,13 @@ public static function makeId()
{
$app = JFactory::getApplication();
+ $registeredurlparams = new stdClass;
+
// Get url parameters set by plugins
- $registeredurlparams = $app->registeredurlparams;
+ if (!empty($app->registeredurlparams))
+ {
+ $registeredurlparams = $app->registeredurlparams;
+ }
// Platform defaults
$registeredurlparams->format = 'WORD';
View
8 libraries/joomla/mail/helper.php
@@ -147,10 +147,16 @@ public static function isEmailAddress($email)
// Check the domain
$domain_array = explode(".", rtrim($domain, '.'));
+
+ // Domain must have at least one country code (except it is localhost)
+ if (count($domain_array) < 2 && $domain_array[0] != 'localhost')
+ {
+ return false;
+ }
+
$regex = '/^[A-Za-z0-9-]{0,63}$/';
foreach ($domain_array as $domain)
{
-
// Must be something
if (!$domain)
{
Something went wrong with that request. Please try again.