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

add more type on codeigniter custom install #487

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

sukrosono
Copy link

No description provided.

Copy link
Member

@niksamokhvalov niksamokhvalov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add tests, please.

@sukrosono
Copy link
Author

Oh yeah, i also want more type. Would you point me the basic test case?

@niksamokhvalov
Copy link
Member

You need to extend basic test case in tests/Composer/Installers/Test/InstallerTest.php.

@sukrosono
Copy link
Author

Yeah, basically I just copy-paste the test case from cake.
this is my first unit test on PHP, It the same as mocking?

@sukrosono
Copy link
Author

Could you make a minor release ASAP after this one merged, please?

Copy link
Member

@niksamokhvalov niksamokhvalov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I don't understand these changes. It's PR include not only new type for Codeigniter, but and changes with additional logic. Can you explain these changes in detail?
  2. Tests in CI is broken.

Sorry, I steel don't merge it.


public function inflectPackageVars($vars)
{
if ($this->matchesVersion('>=', '3.11.1')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... Why used non-existed version? I don't see this version on releases page: https://github.com/bcit-ci/CodeIgniter/releases.

Copy link
Author

@sukrosono sukrosono Jun 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the why
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current version is 3.1.11, but you comparing with 3.11 version. It's error?

@sukrosono
Copy link
Author

  1. You can suggest title changes, in your point of view would be more relevant.
  2. Yes, I am not surprised with PHP 5 Travis failures, I will take a look, but personally I would drop PHP 5 support.

@sukrosono sukrosono changed the title new type on codeigniter add more type on codeigniter custom install Jun 8, 2021
@sukrosono
Copy link
Author

I need your approval to run the Travis build, I am already trying to use my own Travis, but I am giving in to do that options.
If you have some suggestion, let me know.
My future work will depend on this.

Copy link
Member

@niksamokhvalov niksamokhvalov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Tests in CI is broken.
  2. Compare CodeIgniterInstaller::$locations with types description in README.md. They are different.

@sukrosono
Copy link
Author

That's awkward, I don't have free time yet to continue. The failure is not on all PHP versions, just likely to fail on 5.3. I can't dig deeper into the CI, I just rely on manual tests before asking you to merge.

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.

None yet

2 participants