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

W.I.P. Migration document for ValueErrors in PHP 8.0 #163

Draft
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Oct 25, 2020

Generated using: https://gist.github.com/Girgias/3796f6c6aec89a2e31a4d901e2c26083

This is heavily W.I.P. but should be a good template from which to work on as I haven't even checked if the XML is valid according to the DTD (but I'm assuming it is).

Improvements/suggestions to the script are welcomed, and the current idea list is:

  • Fetch the argument name if possible from the stubs
  • Better wording?

@cmb69
Copy link
Member

cmb69 commented Dec 11, 2020

Thank you for the PR! I had a somewhat closer look, and it seems to the script already does a rather good job. A very minor issue that this won't build, because there are a couple of unknown book.* IDs, but these can quickly be fixed manually. The most concerning issue is the vast amount of ValueErrors thrown from within helper functions; this likely needs to be checked manually. As such it may not really make sense to retrieve the argument names from the stubs or reflection.

It seems to me the most reasonable way forward is to run the script on the latest sources, and to apply the necessary manual fixes. Maybe applying a few tweaks upfront makes sense, but we shouldn't wait too long for that part of the changelog. :)

@Girgias
Copy link
Member Author

Girgias commented Dec 13, 2020

So I did some work to try to go up the "call stack" for helper functions, I kept it at a maximum of 10 calls, because I think i've got some cyclic calls going around sooo... going to start cleaning it up manually.

@afilina
Copy link
Contributor

afilina commented Dec 22, 2022

This looks like a big endeavour. If it's too big to get done in one PR, perhaps we can split it into smaller work chunks so we can get something merged, before it diverges too much from master. Or perhaps what we have here is good enough to be merged, and we can make a reminder to address the rest?

@cmb69
Copy link
Member

cmb69 commented Dec 22, 2022

I'm totally unsure how to proceed here. All these migration guide entries also need changelog entries in the manual proper; very few (~ 29) are already there – this is a huge amount of work. Sigh.

@afilina
Copy link
Contributor

afilina commented Dec 22, 2022

My suggestion is to accept that you won't be able to do everything in one go. Merge what you have. Add a note at the top of the page that the list is incomplete, maybe even invite people to a ticket that explains what remains to be done. Perhaps this will help recruit more doc contributors.

@cmb69
Copy link
Member

cmb69 commented Dec 23, 2022

[…] Merge what you have. […]

Generally, I agree, but this PR needs at least some work. Currently, we have, for example:

Screenshot 2022-12-23 163616

@Girgias
Copy link
Member Author

Girgias commented Dec 23, 2022

I could try to spend some time again on this if the Foundation doesn't mind that I spend time on it, will query with them.

@Girgias
Copy link
Member Author

Girgias commented Jan 18, 2023

Okay, got the green light for working on this on Foundation time.

First pass really as other functions likely need to be moved into this section.
And out of the massive still needing to sort standard section
This is going to be such a pain... fml
As far as I can tell we don't throw ValueErrors here
@afilina
Copy link
Contributor

afilina commented Aug 2, 2023

@Girgias I can assist with splitting this into several smaller PRs so that we can start merging things. Let me know if you want to chat about.

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.

3 participants