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

Remove minOccurs attr from WSDL message part #3022

Merged

Conversation

elidrissidev
Copy link
Member

@elidrissidev elidrissidev commented Feb 15, 2023

Description (*)

As mentioned in #1377 (comment), the WSDL message <part> may not contain a minOccurs and it throws an error when calling the endpoint.

Related Pull Requests

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@leissbua
Copy link
Contributor

leissbua commented Feb 15, 2023

Thank you. Hopefully not offending anyone, but the quality of the releases must change and has gotten noticeably worse. Any developer changing the WSDL has to flush WSDL cache, to not repeat killing WSDL again and again without noticing.

@elidrissidev
Copy link
Member Author

No offense taken. I do agree that a lot of bugs slipped through last year, especially after the project got more maintainers and code merging and release velocity increased.

I will do the best from my side to ensure this does not happen again in the future, and I'm sure others will too.

Thanks for reporting!

@kiatng kiatng requested a review from luigifab February 15, 2023 15:17
@fballiano
Copy link
Contributor

are we breaking it yet again now? is this extremely necessary? #1377 was merged in august of last year, it doesn't make sense to me to break the WSDL again to fix something that's in our code since a year.

also, the quality of the releases is as high as we can, as I said many times here, some of us are dedicating MANY hours to this project completely for free, we have ZERO sponsorship and very less help. whoever wants more should participate more. period.

@leissbua
Copy link
Contributor

Thanks for your feedback, i appreciate it. It is a wonder, that we did not hear of the problem. you will not create trouble by fixing a broken WSDL. It needs to be fixed, and no trouble will be created by fixing it. If it is not fixed you will fix it later when people complain. Compalints will start when people flush their WSDL cache, thats why i recommend to flush WSDL whenever working on the WSDL.

"whoever wants more should participate more. period." I understand what you are trying to say, but quality should not be related to money, because quick hacking never succeeds. But i fully agree, there should be sponsorship as MageOne makes profit and could not exist without OpenMage. Create a donation pool and we surely will donate, and feedback to good or bad things is always given.

@fballiano
Copy link
Contributor

quality is related to what we can do, we all give what we can, doing the best that we can. so I'm not accepting that "the quality is low" when our platform is probably the easiest to upgrade. many platforms have more problems than ours (while upgrading) and upgrading with zero effort is some kind of utopia.

anyway I'm checking the WDSL and API systems for my customers (that use this version) and the systems are working fine so I'm not sure I want to break it again. is it creating a practical problem or it's just about the wdsl validation?

@leissbua
Copy link
Contributor

leissbua commented Feb 15, 2023

It is creating a practical problem, every WSDL validator denies the WSDL, as its broken. Use SOAP-UI if you want to test. Setup any ERP if you want to see what problems they have. I really dont want to create trouble, I am a Magento developer since 2008, so i guess i did a few hours here at Cartware GmbH and should be able to tell, not telling that you dont!

"it's just about the wdsl validation", we should set standards high, so "just the wsdl validation" is a bit to easy for my taste. Forgive my directness, but WSDL problems cost us days, while they could have been easily avoided by testing the result manually.

@fballiano
Copy link
Contributor

Sincerely I don't understand everything from your message.

anyway, as you can see, we're already checking your report. my customers have ERPs using SOAP_V2 and they're obviously working, I'm test the WSDL with boomerang and I'm seeing no problem. I don't have SOAP-UI, if you have it you can send a screenshot or something of the error.

since you complain about quality, I'm just making sure that we don't break it again for the people that upgraded and flushed their cache already.

my boomerang client working fine with latest openmage v20:
Screenshot 2023-02-15 alle 17 46 54

@elidrissidev
Copy link
Member Author

Sincerely I don't understand everything from your message.

anyway, as you can see, we're already checking your report. my customers have ERPs using SOAP_V2 and they're obviously working, I'm test the WSDL with boomerang and I'm seeing no problem. I don't have SOAP-UI, if you have it you can send a screenshot or something of the error.

since you complain about quality, I'm just making sure that we don't break it again for the people that upgraded and flushed their cache already.

my boomerang client working fine with latest openmage v20:

There's a typo with wsdl in the URL. Could you try executing the request in which the weight was added?

As far as I can see, removing the attribute won't create any issue since it doesn't belong there in the first place. Whether an error is thrown or not might be client-dependent, so it should be removed.

@fballiano
Copy link
Contributor

the 2 systems I've tested already have the "weight" parameter and I don't see any error.

what do you mean "there's a typo in the WDSL url"?

@elidrissidev
Copy link
Member Author

what do you mean "there's a typo in the WDSL url"?

In the screenshot you shared, there's a typo in the URL (?wdsl)

@fballiano
Copy link
Contributor

probably "boomerang" autoadded the correct wsdl=1 parameter by itself. i didn't test the single endpoint but setting up the test call didn't generate any error and my customer that have the soap system in production are working fine.

if the problem is generated when calling the single endpoint that's another story but not what's been reported.

@fballiano
Copy link
Contributor

also, I'd like to hear from @luigifab since he's the author of the original PR.

@leissbua
Copy link
Contributor

leissbua commented Feb 16, 2023

The discussion is fully pointless, as the XML is simply not valid, I have no clue why time is wasted with discussions.

image

I did not fix this for fun, it was a client issue with Microsoft Axapta, that could be reproduced with SOAP-UI on our side:

Error:
image

Reason:

minOccurs="0"

must not be in a message, as it invalidates the WSDL. It is clearly a mistake created by copy and paste.

Explanation:
In case you did not know, WSDL is not flushed via Magento-Cache flush, but resides in /tmp - so if you did not delete manually you wont see any error, as i stated a few times.

Copy link
Collaborator

@luigifab luigifab left a comment

Choose a reason for hiding this comment

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

I'm sorry for that, but since I using this change, I haven't had any problems.
I updated yesterday and flush all wsdl cache (I think, I hope), all still working.

I'm not sure how to validate openmage wsdl.

@leissbua
Copy link
Contributor

You can validate it at any time, simply via this website:

https://www.wsdl-analyzer.com/

@fballiano fballiano merged commit 40987a5 into OpenMage:1.9.4.x Feb 16, 2023
@elidrissidev elidrissidev deleted the fix/wsdl-message-part-invalid-attr branch February 16, 2023 09:03
elidrissidev added a commit to elidrissidev/magento-lts that referenced this pull request Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Sales Relates to Mage_Sales
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants