Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
  • Nov 04 2021 12:45
    kinglozzer closed #63
  • Nov 04 2021 12:43
    kinglozzer closed #171
  • Nov 04 2021 12:43
    kinglozzer commented #171
  • Nov 04 2021 12:41
    kinglozzer closed #233
  • Nov 04 2021 12:41
    kinglozzer commented #233
  • Nov 04 2021 12:37

    kinglozzer on 3

    Fix travis builds Merge pull request #235 from bi… (compare)

  • Nov 04 2021 12:37
    kinglozzer closed #235
  • Nov 04 2021 10:59
    kinglozzer synchronize #235
  • Nov 04 2021 10:10
    kinglozzer synchronize #235
  • Nov 04 2021 10:01
    kinglozzer synchronize #235
  • Nov 04 2021 09:58
    kinglozzer opened #235
  • Nov 04 2021 09:57
    kinglozzer converted_to_draft #233
  • Nov 04 2021 09:43
    kinglozzer edited #233
  • Nov 04 2021 09:38

    kinglozzer on master

    Update branch alias to 4.x-dev (compare)

  • Nov 04 2021 09:32

    kinglozzer on 3

    (compare)

  • Nov 03 2021 17:37
    kinglozzer commented #233
  • Nov 03 2021 17:36
    kinglozzer synchronize #233
  • Nov 03 2021 17:23
    kinglozzer synchronize #233
  • Nov 03 2021 16:23
    kinglozzer synchronize #233
  • Nov 03 2021 15:50
    kinglozzer synchronize #233
Roman Schmid
@bummzack
do you have these in your require-dev?
"require-dev": {
  "phpunit/PHPUnit": "~3.7",
  "silverstripe/sqlite3": "^1.4",
  "omnipay/dummy": "~2.1",
  "omnipay/paymentexpress": "~2.1",
  "omnipay/paypal": "^2.5",
  "guzzle/guzzle": "^3.9",
  "omnipay/tests": "^2.0",
  "omnipay/common": "~2.5"
},
Morven Lewis-Everley
@mlewis-everley
Hmm, not all those
just updating
Ugh, finally!
looks like it is working now
Roman Schmid
@bummzack
awesome
Morven Lewis-Everley
@mlewis-everley
I think it was the paymentexpress module
Roman Schmid
@bummzack
yeah… when testing modules this way (eg. when they are imported as a dependency) you'd have to add their dev-dependencies to your main composer file
because composer doesn't install these… I'm not sure if there's a flag for this though. Probably :)
Morven Lewis-Everley
@mlewis-everley
that is annoying
I usually just use composer require or composer update on local/dev, then composer install --no-dev on live
Roman Schmid
@bummzack
yeah, but even if you install regularly, it will only install dev requirements of your main composer file
Morven Lewis-Everley
@mlewis-everley
I had never realised that
I did try running composer update --dev but that didn't seem to work
Roman Schmid
@bummzack
no it wont. the dev requirements are only relevant to the library itself, not the global project
I guess that's the reasoning behind that…
no need to install all dev dependencies of all required modules
Morven Lewis-Everley
@mlewis-everley
hmm, any reason this test exists?
apparently it has no assertions?
Roman Schmid
@bummzack
haha. interesting…
Morven Lewis-Everley
@mlewis-everley
Should I remove it?
Roman Schmid
@bummzack
better mark as incomplete I guess
Morven Lewis-Everley
@mlewis-everley
How do I that?
Roman Schmid
@bummzack
$this->markTestIncomplete('getCMSFields tests');
Morven Lewis-Everley
@mlewis-everley
OK, the way I have re-written this does not allow for returning $this->httpError() so easily
so I am adding some more exception types
and throwing exceptions from the functions, then catching them in the action
does that sound ok?
Roman Schmid
@bummzack
@mlewis-everley hmm I'd like to avoid that if possible
let me have a look
why do you need to throw the exceptions? Can't you just return null there and output an error in case the return value is null?
Morven Lewis-Everley
@mlewis-everley
Well you were outputting two different error codes (403 and 404)
Roman Schmid
@bummzack
yes
403 if the intent is invalid
404 if the payment can't be processed or doesn't exist
Morven Lewis-Everley
@mlewis-everley
so just returning null isn't enough to determine what error code to return?
Also, what's wrong with exceptions?
Roman Schmid
@bummzack
It makes the codebase bigger for a problem that can easily be solved with a slightly different architecture
these exceptions aren't being used anywhere else
just to handle stuff between a few protected methods doesn't warrant separate exception classes imho
Morven Lewis-Everley
@mlewis-everley
I don't really see how I can return one of two different error codes based on just returning null from a function?
Roman Schmid
@bummzack
then don't nest the functions? eg. getServiceResponse gets the intent via getPaymentIntent. The latter is the one that should have it's own error handling
you can get the intent, check its return value and respond with an error if it's null
then pass it on to the getServiceResponse method if it's valid
I can give it a go if you want… not sure what's needed that I can push to your pr though
Morven Lewis-Everley
@mlewis-everley
Well, I can revert it back, I don't really see how you are going to reduce the duplication in codebase
Roman Schmid
@bummzack
I'll try to add to your PR in a sec.
Roman Schmid
@bummzack
Lol, i pushed to the wrong repo
oh well, it's merged now :)