fix($observe): check if the attribute is undefined #9720

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@Puigcerber
Contributor

Check if the attribute is undefined before manually applying the function because if not an undefined
property is added to the scope of the form controller when the input control does not have a name.

Closes #9707

#ngEurope /cc @rodyhaddad @vojtajina

@mary-poppins

I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let us know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

@mary-poppins mary-poppins added cla: no and removed cla: yes labels Oct 21, 2014
@Puigcerber
Contributor

Hi @mary-poppins! I have just signed the CLA. Cheers.

@Puigcerber Puigcerber changed the title from fix($observe): check if the attribute is undefined before manually applying the function to fix($observe): check if the attribute is undefined Oct 21, 2014
@Narretz Narretz and 2 others commented on an outdated diff Oct 21, 2014
src/ng/compile.js
@@ -1073,7 +1073,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
listeners.push(fn);
$rootScope.$evalAsync(function() {
- if (!listeners.$$inter) {
+ if (!listeners.$$inter && typeof attrs[key] !== 'undefined') {
@Narretz
Narretz Oct 21, 2014 Contributor

What if the name attribute is set, but empty? Does the condition apply?

@Puigcerber
Puigcerber Oct 21, 2014 Contributor

I've checked it and an empty name does not create a property in the scope.

@rodyhaddad
rodyhaddad Oct 21, 2014 Contributor

How about using attrs.hasOwnProperty(key) instead?

@Puigcerber
Puigcerber Oct 21, 2014 Contributor

You're right. I have updated my commit ;)

@Puigcerber Puigcerber fix($observe): check if the attribute is undefined
Check if the attribute is undefined before manually applying the function because if not an undefined
property is added to the scope of the form controller when the input control does not have a name.

Closes #9707
9d8eac2
@caitp
Contributor
caitp commented Oct 22, 2014

I am a fan of this, lets get this in. @Puigcerber you signed with pablo85@... right?

@caitp caitp added this to the 1.3.x milestone Oct 22, 2014
@caitp caitp self-assigned this Oct 22, 2014
@caitp caitp commented on the diff Oct 22, 2014
test/ng/directive/inputSpec.js
@@ -1331,6 +1331,11 @@ describe('input', function() {
expect(scope.name).toEqual('adam');
});
+ it('should not add the property to the scope if name is undefined', function() {
+ compileInput('<input type="text" ng-model="name" />');
+ expect(scope.form['undefined']).toBeUndefined();
@caitp
caitp Oct 22, 2014 Contributor

I think what we should do though, is spy on the $addControl and $$renameControl methods of the form though... I think we'd have to rewrite it to not use compileInput to make that work

@caitp caitp added a commit that closed this pull request Oct 22, 2014
@Puigcerber @caitp Puigcerber + caitp fix($observe): check if the attribute is undefined
Check if the attribute is undefined before manually applying the function because if not an
undefined property is added to the scope of the form controller when the input control does not
have a name.

Closes #9707
Closes #9720
531a8de
@caitp caitp closed this in 531a8de Oct 22, 2014
@caitp
Contributor
caitp commented Oct 23, 2014

CLA verified by Max Sills and George Lu

@caitp caitp added cla: yes and removed cla: no cla: maybe labels Oct 23, 2014
@Puigcerber
Contributor

Hi @caitp! Sorry I didn't answer before but I was in the conference. It feels good to have contributed to the core for first time and I hope it's not the last. Thanks for merging!

@caitp
Contributor
caitp commented Oct 24, 2014

Right on, thanks for the patch =] Hope to see more of that from you in the future!

@Puigcerber Puigcerber deleted the unknown repository branch Oct 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment