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

Allow Bundle IDs that have a valid prefix - strict validation #2515

Merged
merged 7 commits into from Mar 12, 2019

Conversation

@maksymmalyhin
Copy link
Contributor

@maksymmalyhin maksymmalyhin commented Mar 11, 2019

This is an updated version of #2430 to address comments:

  • strict validation for extension bundle IDs
bstpierr and others added 4 commits Feb 22, 2019
return YES;
}
}
return NO;
}

+ (NSString *)bundleIdentifierByRemovingLastPartFrom:(NSString *)bundleIdentifier {
NSString *bundleIdComponentsSeparator = @".";
Copy link
Member

@ryanwilson ryanwilson Mar 11, 2019

Nit: bundleID instead of Id here and below.

return @"";
}

NSMutableArray<NSString *> *mutableNundleIdComponents = [bundleIdComponents mutableCopy];
Copy link
Member

@ryanwilson ryanwilson Mar 11, 2019

Nit: s/Nundle/Bundle/ here and below.

@@ -45,8 +45,8 @@
+ (NSArray *)relevantURLSchemes;

/**
* Checks if the bundle identifier exists in the given bundles.
* Checks the given bundles to see of them is a prefix of the given identifier.
Copy link
Member

@ryanwilson ryanwilson Mar 11, 2019

Maybe "Checks if any of the given bundles have a matching bundle identifier prefix (removing extension suffixes)."?

@@ -29,6 +29,7 @@ Firebase Core includes FIRApp and FIROptions which provide central configuration
s.private_header_files = 'Firebase/Core/Private/*.h'
s.framework = 'Foundation'
s.dependency 'GoogleUtilities/Logger', '~> 5.2'
s.dependency 'GoogleUtilities/Environment', '~> 5.2'
Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Mar 11, 2019

@ryanwilson Not sure we catch missing dependencies in our tests...

Copy link
Member

@ryanwilson ryanwilson Mar 11, 2019

Nice catch - I'm not sure why this wouldn't be caught by pod lib lint - @paulb777 any ideas?

Copy link
Member

@paulb777 paulb777 Mar 11, 2019

Copy link
Member

@ryanwilson ryanwilson Mar 11, 2019

Makes sense! Can you please move it up one to be above Logger (alphabetical order)?

@maksymmalyhin maksymmalyhin self-assigned this Mar 11, 2019
Copy link
Member

@ryanwilson ryanwilson left a comment

LGTM with one small comment.

@@ -29,6 +29,7 @@ Firebase Core includes FIRApp and FIROptions which provide central configuration
s.private_header_files = 'Firebase/Core/Private/*.h'
s.framework = 'Foundation'
s.dependency 'GoogleUtilities/Logger', '~> 5.2'
s.dependency 'GoogleUtilities/Environment', '~> 5.2'
Copy link
Member

@ryanwilson ryanwilson Mar 11, 2019

Makes sense! Can you please move it up one to be above Logger (alphabetical order)?

@maksymmalyhin maksymmalyhin merged commit e53d552 into master Mar 12, 2019
2 checks passed
@maksymmalyhin maksymmalyhin deleted the mm/bundleprefix branch Mar 25, 2019
@firebase firebase locked and limited conversation to collaborators Oct 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants