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

Drop "simple" attribute from references in XML schema #1895

Merged
merged 2 commits into from
Nov 25, 2018

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Nov 16, 2018

Q A
Type improvement
BC Break yes
Fixed issues #1854

Summary

The simple option for references has been dropped in favour of the storeAs directive, so it makes sense to drop this from the schema. This PR also updates the wording in some exception messages to no longer refer to these references as "simple" but rather "identifier references".

@alcaeus alcaeus added this to the 2.0.0 milestone Nov 16, 2018
@alcaeus alcaeus self-assigned this Nov 16, 2018
@alcaeus alcaeus requested a review from malarzm November 16, 2018 06:38
@malarzm
Copy link
Member

malarzm commented Nov 16, 2018

Tests are unhappy with your change ;)

@alcaeus
Copy link
Member Author

alcaeus commented Nov 16, 2018

Meh, took a gamble on those exception messages. Who’d think we actually check them in tests 😛

@malarzm
Copy link
Member

malarzm commented Nov 16, 2018

@alcaeus
Copy link
Member Author

alcaeus commented Nov 25, 2018

Tests fixed 👍

@malarzm
Copy link
Member

malarzm commented Nov 25, 2018

Awesome! 👍

@malarzm malarzm merged commit 31d7aef into doctrine:master Nov 25, 2018
@etshy
Copy link
Contributor

etshy commented Jan 2, 2019

Just to let you know about this change.
Seems IDEs (PhpStorm at least) don't like the <!-- deprecated --> comment after the element.
When I map Documents with XML PhpStorm warn me the store-as attribute is deprecated.

I don't know if it comes only from how Phpstorm is reading the xsd file or if the <!-- deprecated --> comment should be before the element that's deprecated.

@jmikola
Copy link
Member

jmikola commented Jan 2, 2019

@etshy: Would you mind submitting a quick PR to move the comment on its own line above the "simple" attributes? Feel free to link back to this PR in your description for added context.

According to https://stackoverflow.com/q/1950075/162228, there isn't a standardized way to mark elements in an XSD as deprecated. It does seem like PhpStorm is picking up on the comment and applying it to the next element it sees; however, I wasn't able to find any reference to that behavior online. The SO thread above also suggests using an xs:documentation element in lieu of a <!--, which might suppress PhpStorm's warning altogether, although it sounds like it might actually be helpful if we can get it to trigger on the "simple" attribute instead of "store-as".

@etshy
Copy link
Contributor

etshy commented Jan 2, 2019

I just realized my problem is not about this PR but surely a much older one... as this one remove completely the simple attribute support.

Do I still make a PR to 1.3 branch, or there is no need ?

Edit :
I found [https://www.gtk.org/gtk-doc/devhelp2.xsd.html#attribute_deprecated](this link) about deprecation in XSD but it doesn't seem to be detected in PHPStorm (and so doesn't work either).

Nothing else

@malarzm
Copy link
Member

malarzm commented Jan 2, 2019

I'd say that PR to 1.3 would still be great, better to have proper attribute shown as deprecated in PHPStorm. If it'd cause troubles in some other editors, we can revisit when it does happen

@jmikola
Copy link
Member

jmikola commented Jan 2, 2019

I just realized my problem is not about this PR but surely a much older one... as this one remove completely the simple attribute support.

Ah, I misread the diff as well. Looks like the comment originated in 1.1.0 with 3b4851c. A 1.3 release is still forthcoming, so I don't see any reason you can't submit a PR against that branch.

@alcaeus alcaeus deleted the drop-simple-xml-references branch January 7, 2019 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants