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

Refactor internal data store of LocalFeaturedImage $source property to not include directory information #924

Merged
merged 16 commits into from
Feb 5, 2023

Conversation

caendesilva
Copy link
Member

Prior to this refactor, the $source property of LocalFeaturedImage classes was forced to start with _media/, and threw an exception if it did not. The value was then normalized to the desired format upon accessing the value.

This PR changes so that the internal data value no longer contain the path information, as it is instead prepended when accessing the value. To keep the internal state consistent, any leading _media/ prefixes are removed. Thus this change is entirely unbreaking as no usage outside the class is affected since the public accessors still function the same.

This change is important from an internal perspective however, as is more compatible with customizable media directories #920

@what-the-diff
Copy link

what-the-diff bot commented Feb 5, 2023

  • The setSource method in the LocalFeaturedImage class was changed to remove a check for _media/ from the beginning of $source.
  • A new test case was added that checks if an exception is thrown when constructing a LocalFeaturedImage with invalid source information (i.e., not starting with '_media').

Note from author: The AI comment below is just wrong. Please ignore it.

  • In FeaturedImageTest, there are two changes:
    • One change removes an existing test case which checked whether or not exceptions were being thrown by passing invalid data into constructors for both classes; this has been replaced by more specific tests cases in each class's respective file (see Restore master project #1 and Merge Hyde/Framework into packages/framework #2).
    • Another change adds another test case to ensure that getContentLength() returns 0 on empty files instead of throwing errors as it did before (Remove Undue Complexity #4) . This also ensures that we don't have any unexpected behavior due to PHP 7+'s stricter type checking rules regarding return values from functions returning int types vs nullable int types like they used to be able too prior to version 7+.

@codecov
Copy link

codecov bot commented Feb 5, 2023

Codecov Report

Merging #924 (1c295d5) into master (e796bb9) will not change coverage.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##              master      #924     +/-   ##
=============================================
  Coverage     100.00%   100.00%             
- Complexity      1237      2474   +1237     
=============================================
  Files            145       290    +145     
  Lines           3269      6538   +3269     
=============================================
+ Hits            3269      6538   +3269     
Impacted Files Coverage Δ
...amework/Features/Blogging/Models/FeaturedImage.php 100.00% <ø> (ø)
...rk/Features/Blogging/Models/LocalFeaturedImage.php 100.00% <100.00%> (ø)
...mework/src/Framework/Actions/StaticPageBuilder.php 100.00% <0.00%> (ø)
...es/framework/src/Support/Concerns/Serializable.php 100.00% <0.00%> (ø)
develop/packages/framework/src/Pages/HtmlPage.php 100.00% <0.00%> (ø)
...k/src/Framework/Views/Components/LinkComponent.php 100.00% <0.00%> (ø)
...rk/src/Console/Commands/PublishHomepageCommand.php 100.00% <0.00%> (ø)
...src/Framework/Exceptions/FileNotFoundException.php 100.00% <0.00%> (ø)
...ackages/framework/src/Console/Concerns/Command.php 100.00% <0.00%> (ø)
...ackages/framework/src/Markdown/Models/Markdown.php 100.00% <0.00%> (ø)
... and 137 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@caendesilva caendesilva marked this pull request as ready for review February 5, 2023 20:15
@caendesilva caendesilva merged commit b69fd2e into master Feb 5, 2023
@caendesilva caendesilva deleted the refactor-image-models branch February 5, 2023 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants