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

Minor services naming fix #10845

Merged
merged 3 commits into from Nov 8, 2018

Conversation

Projects
None yet
5 participants
@sarjon
Member

sarjon commented Oct 4, 2018

Questions Answers
Branch? 1.7.5.x
Description? When looking at HtaccessFileChecker and RobotsTextFileChecker they seem to be checking if file is writable, but interface is a bit misleading. So it updates the naming.
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? n/a
How to test? No behavior has changed. Page "Configure -> Shop parameters -> Traffic & Seo -> Seo & Urls" should work as before.

This change is Reviewable

@sarjon

This comment has been minimized.

Member

sarjon commented Oct 4, 2018

@matks wdyt?

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Oct 4, 2018

You can't change file names and methods names without introduce BC break. Do you know when these classes were added?

@sarjon

This comment has been minimized.

Member

sarjon commented Oct 4, 2018

without introduce BC break

yes, they should be part of 1.7.5, thats why i want to change them before release. :)

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Oct 4, 2018

In isWritable methods.

return is_writable(dirname($filePath));

can be:

return is_writable($this->rootDirectory);

Prevent useless usage of dirname().
Otherwise I approved!

@sarjon

This comment has been minimized.

Member

sarjon commented Oct 4, 2018

Prevent useless usage of dirname().

totally right, i intended to do this, but missed somehow. :(

@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Oct 4, 2018

And I just realize these classes have same methods, could be useless to have an abstract class? Wdyt?

@matks

This comment has been minimized.

Contributor

matks commented Oct 9, 2018

@PierreRambaud Well, except for one string these classes are identical.
So why even an abstract class ? We make create a class FileChecker and we can

  • construct 2 services by proving the file name as constructor input
    or
  • construct 1 service and provide the filepath as argument in isWritable($filepath)

@matks matks added the migration label Oct 12, 2018

@sarjon

This comment has been minimized.

Member

sarjon commented Nov 7, 2018

i've implemented single specialized service instead of 2 generic now.

ping @PierreRambaud @matks for review. :)

if (file_exists($filePath)) {
return is_writable($filePath);
}
return is_writable(dirname($filePath));
return is_writable($this->fileDir);

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 7, 2018

Contributor

Something I don't understand with this function:
If the file doesn't exists, why checking for writable directory?
Having a test on .htaccess should test the .htaccess, not the parent directory :/

This comment has been minimized.

@sarjon

sarjon Nov 7, 2018

Member

if file does not exist, but its directory is writable, we can say that file is writable (we dont care whether it exists or not, we know that it's writable).

but this is my guess. :)

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 7, 2018

Contributor

I understand what you mean but watchout with linux permissions (sticky bit and acls), the file doesn't exists, so it's logically not writable :)

This comment has been minimized.

@sarjon

sarjon Nov 7, 2018

Member

well, if it was working in legacy and nobody raised an issue, i guess its okay? :)

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 7, 2018

Contributor

PrestaShop must be compatible with complex architecture, this is maybe something we will have to change in the future :)

@matks

matks approved these changes Nov 7, 2018

@matks matks added the waiting for QA label Nov 7, 2018

@marionf marionf added QA ✔️ and removed waiting for QA labels Nov 8, 2018

@matks

This comment has been minimized.

Contributor

matks commented Nov 8, 2018

Thank you @sarjon

@matks matks added this to the 1.7.5.0 milestone Nov 8, 2018

@matks matks merged commit f6e5a5d into PrestaShop:1.7.5.x Nov 8, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment