| gaborhojtsy |
his sample issue is at https://www.drupal.org/project/drupal/issues/3023981 |
| mikelutz |
In line with that is a renewed focus at clearing out the skipped deprecation list too. I have half a dozen in migrate land on that list that I still need to finish taking care of. |
| gaborhojtsy |
@mikelutz let’s keep that a separate topic, this is more a policy / process thread AFAIS |
| mglaman |
I think it’s good to have a CR for each deprecation so that people know how to update. CR is probably most appropriae since we don’t have a “Using the entity repository” doc |
| mglaman |
or “Entity repository v. Entity Storage” |
| xjm |
Most but not all deprecations should have a CR |
| mikelutz |
Fair enough, Then on the policy/process part of it, I want to bring up deprecated constants. AFAIK we don’t have a way to trigger an error on them, so I’m wondering how we ensure we’ve removed their usage and don’t reintroduce them. I know @mglaman has been working on the phpstan which might help there. |
| xjm |
@mikelutz could propose a separate topic for that? |
| xjm |
But @gaborhojtsy what is the question here? |
| xjm |
Just "Let's please finalize the coding standards issue [link] and finish the phpcs rules"? |
| gaborhojtsy |
@berdir raised this one, AFAIS:
- define/document/state policy on which deprecations need CR and tests
- finish https://www.drupal.org/project/coding_standards/issues/3024461 so people can use it :slightly_smiling_face: |
| mikelutz |
Ah fair enough then. I was thinking this thread was more for deprecation policy and process in general. |
| xjm |
So for me, tests and CR are "Should" and should only be excluded on a case-by-case basis at committer discretion |
| xjm |
99% of the issues I commit, I require a CR |
| xjm |
and maybe 95% a test |
| xjm |
Some are not testable, or just really really not worth testing |
| xjm |
Some internal deprecations don't need a CR and the message is enough, but I usually ask for one |
| berdir |
so, there are two specific examples in there |
| berdir |
\Drupal\Core\Entity\EntityReferenceSelection\SelectionTrait |
| berdir |
that is a trait that consists of only a constructor, create() method and a bunch of properties |
| berdir |
two classes use it |
| berdir |
in that issue, I'm deprecating it because the constructors of those two classes change and are no longer identical |
| berdir |
the trait was only added a year or so ago |
| berdir |
chances that anyone else is using it is basically 0 :slightly_smiling_face: |
| xjm |
I would still say it should have a CR and test unnless it was added since 8.6.0 |
| xjm |
Or unless it's really onerous to test or something |
| berdir |
but what would the CR say? there is no 1:1 replacement, you simply have to write your own constructor now |
| xjm |
Yes, that would be what the CR says :slightly_smiling_face: |
| berdir |
and a test would require a test class that uses that trait and then we'd need to instantiate it.. but it doesn't actually *do* anything |
| berdir |
right, but the trigger_error() already does that :slightly_smiling_face: |
| xjm |
I still think it should have a CR since it's public API, but a CR is easy to add |
| xjm |
However if it's not testable then that's something that the committer could sign off on review |
| berdir |
well, it would not be that complicated, but it's something that we'll have to remove again in a year |
| berdir |
the second example are constructors |
| berdir |
technically, strictly following the bc policy, we can just change them |
| berdir |
but we broke quite a few contribs over the last few minors |
| xjm |
Yeah, I have been pushing back more on being careful with constructor changes |
| berdir |
so we found a acceptable compromise that makes those optional, with a @trigger_error() and so on, I think that works quite well |
| berdir |
but in my entity manager issues, I'm updating *dozens* of constructors like that |
| xjm |
We could actually add the example for the constructors to the policy doc |
| xjm |
the internal API policy I mean |
| berdir |
yes, I wanted to create an issue to clarify that |
| xjm |
or well the deprecation policy and update the other referencing it |
| berdir |
yep, but about CR and tests.. |
| xjm |
So tests for a constructor change should be easy right? Just a unit test |
| xjm |
Is the concern that it's "dozens of unit tests"? |
| berdir |
having legacy tests for all those constructor changes would probably double those already large patches (the one referenced here is 180kb and that's just a tiny part of the whole thing |
| berdir |
and it would be a ton of tedious work |
| xjm |
I think we can write one CR summarizing and include that in them |
| xjm |
The CR can say it's an intermediate layer |
| xjm |
That's all good reason to _have_ a CR |
| berdir |
the entity manager related constructor changes all point to the main/original entity.manager CR: https://www.drupal.org/node/2549139 |
| xjm |
For the tests, we've not required them before for constructor changes, but OTOH we haven't done a lot of the deprecations before, so I dunno on that. The risks would be that it stops working between now and a year from now if we do not add tests. |
| berdir |
but that doesn't actually list all the constructors yet |
| berdir |
yes, but in a year, we already have to delete all those tests again in 9.x :wink: |
| berdir |
and we have been doing a lot of them |
| berdir |
there are 40 lines in core that mention construct and that CR |
| berdir |
and there will be a lot more by the time I'm done |
| xjm |
Well, the tests would stay around inn 8.9 until 2021 |
| xjm |
I feel like the existing CR doesn't really cover the constructor changes at all |
| berdir |
and I don't understand the benefit of a CR that lists 100 constructors |
| berdir |
this isn't a common API that a lot of modules use, many of them will likely not have been subclassed at all |
| berdir |
there are a few exceptions, when it is actually base classes.. we mentioned ContentEntityForm there, and it makes sense to mention other base handlers too |
| xjm |
Could we add language that summarizes the type of change to the constructors and an example? |
| berdir |
the advantage of @trigger_error() is that the 0.1% of all the modules that are affected are actively notified and not everyone has to read a 100 item list |
| berdir |
yup, that would make sense |
| berdir |
saying that as part of that, a lot of service/forms/plugin constructors are changing, that EntityManager is replaced with 1-N new dependencies and people who review their EntityManager uses and update them accordingly |
| berdir |
to be completely honest, I quite enjoy doing those updates, but I have more fun things to do in my evenings than writting 50loc unit tests for every constructor change that I'm doing in these issues :slightly_smiling_face: |
| xjm |
For the tests question, will anything else be exercising the deprecated code paths? What are the consequences if they break and how obvious of a break would it be? |
| berdir |
and I feel like we have to weight the cost of breaking something/not having perfect update update instructions for those rare cases vs. someone actually having to do the work |
| berdir |
nothing will in core, but per the current policy, I'd say constructor BC is a best-effort thing. it's only the BC-part that could possibly be broken, the rest is executed |
| berdir |
we're talking about snippets like this:
```
if (!$entity_field_manager) {
@trigger_error('The entity_field.manager service must be passed to CommentManager::__construct(), it is required before Drupal 9.0.0. See https://www.drupal.org/node/2549139.', E_USER_DEPRECATED);
$entity_field_manager = \Drupal::service('entity_field.manager');
}
$this->entityFieldManager = $entity_field_manager;
``` |
| berdir |
in a _construct() |
| berdir |
about the only thing that could be broken is getting the wrong service, or maybe a typo in a deprecation message (mentioning the wrong class or so) |
| berdir |
if a module really gets broken by that, it can create an issue or just update its own constructor |
| larowlan |
I'm ok with no deprecation tests for those but we need to update the 'how we deprecate' policy |
| berdir |
I'll create an issue with a proposal, promised that a few times now, time that I actually do something :wink: |
| dww |
I'm also interested in the proper way to change constructors. I'm coming here via https://www.drupal.org/project/drupal/issues/2492171 -- both that and https://www.drupal.org/project/drupal/issues/2940383 propose to change the constructor for FileUploadResource, a rest plugin of file.module. I guess we should make the new constructor args optional and add the trigger_error() from @berdir's snippet above, instead of simply dying if someone calls this constructor without the new args. Is that the basic idea? I'm down with that. I'd love to not break anyone's code for this, as unlikely as extending FileUploadResource probably is. |
| berdir |
that is the idea. also, if both issues land in the same minor release, we don't need BC *between* them |
| berdir |
only between 8.6 and final 8.7 |
| berdir |
in case they are actually conflicting changes and not just both adding extra arguments |
| dww |
@berdir like so? https://www.drupal.org/project/drupal/issues/2492171#comment-12956210 |
| dww |
https://www.drupal.org/files/issues/2019-02-04/2492171.217_218.interdiff... is probably more interesting link. :wink: |
| berdir |
Finally created the issue that I promised a few times: https://www.drupal.org/project/drupal/issues/3030640 |
| mglaman |
@mikelutz I’ve added some basic testing integration, so we cold start asserting deprecated constants |
| gaborhojtsy |
@mglaman first, amazing work! I’ve seen Dries already used it to update his custom code :smile: https://twitter.com/Dries/status/1092145815521505280 |
| mglaman |
:smile: and Dries has convinced me to make a more streamlined process |
| gaborhojtsy |
@mglaman yeah anything to make it easier to run is going to help :slightly_smiling_face: |
| mglaman |
Also, for any feedback on the process as a whole (as it merges phpstan, phpstan-drupal, and deprecation rules) issues could go https://github.com/mglaman/drupal-deprecation-testing |
| gaborhojtsy |
@mglaman the blog post itself is a great step :slightly_smiling_face: |
| mglaman |
that’s where I put @berdir’s request to ignore AssertLegacyTrait |
| gaborhojtsy |
@mglaman are there known limitations where this tool will not find deprecations? (other than running out of memory)? ie. is this the holy grail of deprecation finding other than the startup cost? |
| mglaman |
AFAIK this is the holy grail as it has a more verbose AST than, say, phpcs. |
| mikelutz |
I really need to spend some time looking at what you did, @mglaman I feel like it may be about the only way to check on constants. |
| mglaman |
I have not tried psalm. |
| mglaman |
Psalm and PHPStan try to “boot” the code and act as if compiled |
| mglaman |
Startup cost can be improved, too, if I add some trickery. |
| mglaman |
Already this has caused some good bugs in phpstan-drupal to be raised, and fixed. The biggest hurdle was bootstrapping Drupal and I have ideas on how to improve that (without waiting for core to start properly dumping an autoloader) |
| gaborhojtsy |
@mglaman well, I meant startup cost as in setting it up, but the travis part can be ignored for one-off runs |
| mglaman |
ah yeah. the next step would be:
something like `./bin/drupal-check` which runs it all |
| alexpott |
phpstan can’t find deprecations which are about a specific code path |
| gaborhojtsy |
@alexpott can you elaborate? |
| mglaman |
@alexpott ^ I’m not understanding |
| alexpott |
For example where a setting is deprecated like https://www.drupal.org/project/drupal/issues/3030501 |
| alexpott |
phpstan is static analysis so for example all the constructor deprecations - which are really additions - does it detect them? |
| mglaman |
I would need to specifically test (I dunno) but would we also need a test case where Drupal’s configured for reverse proxy? |
| mglaman |
Right now my test run is pretty basic and doesn’t tweak settings.php which would cause some things to turn on/off |
| alexpott |
Or for example the deprecation in \Drupal\Core\Config\ExtensionInstallStorage::__construct |
| mglaman |
If nothing is calling it null it wn’t catch |
| mglaman |
and it pulls in the services definition, which has
```
config.storage.schema:
class: Drupal\Core\Config\ExtensionInstallStorage
arguments: ['@config.storage', 'config/schema', '', true, 'NaNnstall_profile%']
``` |
| mglaman |
so it’s def not passing in a valid value, but it’s not null |
| gaborhojtsy |
runtime logging or testing would not catch it either then |
| gaborhojtsy |
testing makes a code path happen so would satisfy phpstan as well |
| alexpott |
(it is passing a valid value NaNnstall_profile% is a container parameter) |
| gaborhojtsy |
defining the boundaries of phpstan’s capability would be quite important :slightly_smiling_face: |
| mglaman |
(I meant phpstan, when it tries to fetch it from the mocked service container definitions) |
| alexpott |
runtime logging does catch these - we’re trigger 1000s of `Install profile will be a mandatory parameter in Drupal 9.0.` |
| alexpott |
per test run. |
| mglaman |
what’s calling the service without it? due to services.yml not having a value? |
| mglaman |
@alexpott I opened up https://github.com/mglaman/drupal-deprecation-testing/issues/2
Just to track and see if we can make it caught |
| mglaman |
the phpstan integration needs more automating testing (finally unblocked that) |
| alexpott |
This is just a limit of static analysis. It can’t discover everything. If it could then it’d be running the code :slightly_smiling_face: with all possible inputs. |
| mglaman |
:slightly_smiling_face: this is true. But I don’t mind trying to write an assertion to at least prove/disprove |
| xjm |
Nothing in the critical path |
| xjm |
Our timeline is too short |
| xjm |
It would need to be a nice-to-have thing or tooling |
| gaborhojtsy |
there are a bunch of “Port $module_name to Drupal 8” ideas |
| alexpott |
What would be really nice is if some of our more find and replacy type deprecations could come with a codeshift/codemod - ie. https://packagist.org/packages/atanamo/php-codeshift / https://packagist.org/packages/atanamo/php-codeshift |
| alexpott |
Unfortunately what prevents that is the fact we still have not automated all of our coding standards. Because we need to be able to convert the PHP to an Abstract Syntax Tree representation and then back to PHP. |
| gaborhojtsy |
so maybe to automate the coding standards would be a better goal then? :slightly_smiling_face: |
| xjm |
Well actually automatinng coding standards checking requires DA/infra work, which has been the blocker |
| xjm |
OCTO tried (and failed) to move that forward |
| xjm |
However I would also say putting the deprecation rule in phpcs is in the critical path and should be done by core devs, and sooner |
| larowlan |
https://github.com/grom358/pharborist does PHP to AST then back to PHP |
| larowlan |
he wrote it with phenaproxima for drupal_module_upgrader |
| larowlan |
its a jquery like syntax for querying and updating php via ast |
| alexpott |
Stuff takes a long time :disappointed: … +1 to getting the deprecation rule in. But that will mean addressing things like format_string() |
| larowlan |
could we do a gsoc about d9 drupal_module_upgrader? |
| alexpott |
@larowlan it’d be great if we could require an AST transform with deprecations where possible. But I feel the coding standards issue will make it too noisy. |
| gaborhojtsy |
https://twitter.com/grasmash/status/1090954772709027841 |
| gaborhojtsy |
like this @larowlan ^ ?:) |
| larowlan |
yeah like that |
| alexpott |
PHPCS fixers are not the nicest things to write unfortunately. |
| larowlan |
yeah, pharborist is much cleaner |
| gaborhojtsy |
I don’t think the base tool matters |
| larowlan |
yeah |
| larowlan |
added https://groups.drupal.org/node/534703#deprecations |
| gaborhojtsy |
@larowlan wohoo, I was about to ask :slightly_smiling_face: |
| gaborhojtsy |
@larowlan sent that in response to @grasmash’s tweet |
| larowlan |
cheers |
| mglaman |
Just making sure, every other monday the meeting is at this time, deprecation office hours are their own meeting, correct? |
| gaborhojtsy |
@mglaman yup, exactly |
| mglaman |
@gaborhojtsy thanks, I want to focus MidCamp sprinters on this that are looking for things to jump into. Will keep meeting time in mind. |
| nerdstein |
Do you all have a sense of any enhanced SimplyTest needs? |
| alexpott |
https://www.drupal.org/project/drupal/issues/3030501 brings up an interesting issue. What do we do when Symfony removes functionality? Syfony 4 does not support custom trusted proxy headers. If we officially support Sf4 on Drupal 8 what does that mean for us? |
| xjm |
@mikelutz wanted to raise something else around deprecating constants and there was also something about Migrate? |
| alexpott |
Deprecated constants are one area where PHPStan and PHPCS can really help. |
| larowlan |
@mglaman it would be great to run your tool against the top 100 contrib modules and open issues for d9/deprecations |
| gaborhojtsy |
@alexpott if we can add our own implementation to wrap their, that would be the only way to keep our functionality, right? |
| larowlan |
yeah deprecated constants is a real issue |
| mglaman |
@larowlan that can be done. I considered donating a Jenkins server for it. Or I guess just running TravisCI. But feels kind of like abusing their service |
| alexpott |
@gaborhojtsy wrapping the Symfony request object would open old arguments. There was tonnes of debate about that long ago. |
| gaborhojtsy |
@nerdstein no immediate changes required for sure :slightly_smiling_face: eventually contrib versioning can become interesting, either d.o will support semantic versioning or d9 may work with 8.x modules or D8 may work with 9.x modules, etc :slightly_smiling_face: |
| gaborhojtsy |
@nerdstein so how simplytest infers core version numbers, etc. will need to adopt, but that is somewhat farther out |
| gaborhojtsy |
@alexpott well, if they removed stuff we need, then not much else we can do… |
| nerdstein |
Perfect ok |
| gaborhojtsy |
@nerdstein the current 9.x branch is a lie, we do everything in 8.x.y for now :slightly_smiling_face: |
| alexpott |
@gaborhojtsy this is the thing though - we only need it if we try to do major version updates in Drupal 8. It is totally okay to drop support for things from 8 to 9 - right? |
| gaborhojtsy |
@alexpott so long as we don’t actually need or want to provide the the feature yeah |
| alexpott |
I would say we don’t want to provide the feature. |
| gaborhojtsy |
in that case, easily removed in 8 to 9 and we can ignore the deprecation since 9 will update to Symfony 4 |
| larowlan |
@mglaman it feels like a good task for a sprint - pick a module, run the tool, open issues |
| xjm |
Seattle? |
| xjm |
Great mentored sprint table idea actually |
| mglaman |
@larowlan yeah. I’ll get some more jobs set up |
| mglaman |
I suppose I could just expand the current drupal-deprecation-testing one. I have one just for commerce, but that was to demo |
| gaborhojtsy |
@Mile23 and @berdir were raising concerns that people will invade issue queues and the timing of making deprecations, etc. — I plan to blog about that soon, was hoping to be able to refer back to @mglaman’s post :slightly_smiling_face: |
| mglaman |
TracisCi limits to 200 in matrix |
| alexpott |
Looking at the Symfony issue - they claim to have done the research and found that the feature was not needed. The only reverse proxies I’ve use used the headers symfony supports or had the ability to set headers anyway you liked. So from my experience we don;’t need the feature. |
| gaborhojtsy |
@alexpott fine with me :slightly_smiling_face: |
| berdir |
@gaborhojtsy yeah, a blog post that describes how to do that so that it actually helps contrib maintainers vs causing more work would be extremely helpful. people are opening a *lot* of issues that just update one file or one instance or something. would be great if I could just post a quick link to a blog post |
| berdir |
I imagine it should start with an issue that provides the make-deprecations-fail patch and then likely discuss together with the maintainer how to split thins up... smaller modules might be fine with a single issue, big ones should be split. |
| mglaman |
Case and point, bojanz opened this: https://www.drupal.org/project/commerce/issues/3030374
ConfigurablePluginInterface is 8.7.x only. So there’s going to be a lot of spam before things can even merge |
| mglaman |
@larowlan :stuck_out_tongue: is there a nice easy “list” of the top 100 or just go to the modules page |
| berdir |
@mglaman yeah, for contrib, it should probably run against latest stable core version :slightly_smiling_face: |
| mglaman |
I’ll make a setup like `drupal-deprecation-testing` but for the top 100 contrib. I feel bad about abusing TravisCI, so I’ll see about setting up a Jenkins server for it. |
| gaborhojtsy |
@mglaman wow :bow: |
| mglaman |
It’d be nice to run the top 100 against stable core + dev |
| mglaman |
:stuck_out_tongue: but that’s 200 jobs |
| larowlan |
@mglaman yeah there was one in the recent bug bounty, let me grab you a link |
| larowlan |
@mglaman https://docs.google.com/spreadsheets/d/1KLfh7nbLb-XSMf7c59Wckg2xUH5Z1ko4... |
| mglaman |
gr8 |
Comments
Comment #2
gábor hojtsyComment #13
justafishComment #14
justafish