Skip to content
This repository has been archived by the owner on Apr 28, 2024. It is now read-only.

Add blacklist functionality per scheme #9

Closed
wants to merge 5 commits into from

Conversation

fembuelita
Copy link

Key changes

Summary

Each scheme can blacklist file extensions in a fairly simple way:
$locator->addBlacklistedExtension($scheme, $extension)
where $scheme is a string representing the scheme and $extension is a string representing a file type (case-insensitive) (do not include the leading dot).

Additionally, blacklist items can be removed with:
$locator->removeBlacklistedExtension($scheme, $extension)

And a set of blacklisted extensions per scheme can be retrieved with:
$locator->getBlacklistedExtensions($scheme)

Unit tests have been added for the new methods and all tests are passing.

@codecov
Copy link

codecov bot commented Apr 16, 2019

Codecov Report

Merging #9 into master will decrease coverage by 0.96%.
The diff coverage is 88.88%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master       #9      +/-   ##
============================================
- Coverage       100%   99.03%   -0.97%     
- Complexity      123      132       +9     
============================================
  Files             4        4              
  Lines           289      312      +23     
============================================
+ Hits            289      309      +20     
- Misses            0        3       +3
Impacted Files Coverage Δ Complexity Δ
src/ResourceLocator.php 98.64% <88.88%> (-1.36%) 98 <8> (+9)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79924a5...e380ceb. Read the comment docs.

@codecov
Copy link

codecov bot commented Apr 16, 2019

Codecov Report

Merging #9 into master will decrease coverage by 0.96%.
The diff coverage is 88.88%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master       #9      +/-   ##
============================================
- Coverage       100%   99.03%   -0.97%     
- Complexity      123      132       +9     
============================================
  Files             4        4              
  Lines           289      312      +23     
============================================
+ Hits            289      309      +20     
- Misses            0        3       +3
Impacted Files Coverage Δ Complexity Δ
src/ResourceLocator.php 98.64% <88.88%> (-1.36%) 98 <8> (+9)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79924a5...d47bb07. Read the comment docs.

@lcharette
Copy link
Member

As stated in userfrosting/UserFrosting#965, I think this repo should also support a whitelist for future use.

I'm not sure on setting up the blacklist on the Locator object compared to the Stream object? It does make more sense to be on the locator side, but it's tied to the stream. Could it be a new argument to findResources/getResources ? Or a new method? Or applied to the result, similar to how eloquent can daisy chain db instructions?

Plus style issues and Codecoverage issue will need to be fixed before merging.

@lcharette lcharette added the enhancement New feature or request label Apr 16, 2019
@lcharette lcharette changed the base branch from master to develop June 22, 2019 00:17
lcharette added a commit that referenced this pull request Jun 22, 2019
@lcharette
Copy link
Member

lcharette commented Jun 22, 2019

I've gone through those proposed changes and thought about it for a while. First, using a blacklist might not be the best option compared to a whitelist. Let's say you only want to return images. Then you have to blacklist all other extensions except jpg and png, otherwise you'll end up with a .xyz in there (and even so, you need to blacklist all possible options, ie .theworldisonfire).

Next, it would be more logical to have the whitelisted extension on the stream level. This cause a problem here as you need the stream object, and all we have is $scheme which is a string.

In the end, I don't think the extension whitelist feature is that important for URI to have. Filtering the result from URI can be done afterward, like it was done here. Furthermore, Some context might need more complex filtering, for example by mimeType or size.

The typo fix were manually merged into develop however.

@lcharette lcharette closed this Jun 22, 2019
@fembuelita
Copy link
Author

Thanks @lcharette for your thorough review! I'm inclined to agree with you.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants