The config.file_watcher option still exists but it's no longer included
in the default environtment file. Since we don't use it, we're removing
it.
The config.assets.assets.debug option is no longer true by default [1],
so it isn't included anymore.
The config.active_support.deprecation option is now omitted on
production in favor of config.active_support.report_deprecations, which
is false by default. I think it's OK to keep it this way, since we check
deprecations in the development and test environments but never on
production environments.
As mentioned in the Rails upgrade guide, sprockets-rails is no longer a
rails dependency and we need to explicitly include it in our Gemfile.
The behavior of queries trying to find an invalid enum value has changed
[2], so we're updating the tests accordingly.
The `favicon_link_tag` method has removed the deprecated `shortcut`
link type [3], so we're updating the tests accordingly.
The method `raw_filter` in ActiveSupport callbacks has been renamed to
`filter` [4], so we're updating the code accordingly.
[1] https://github.com/rails/rails/commit/adec7e7ba87e3
[2] https://github.com/rails/rails/commit/b68f0954
[3] Pull request 43850 in https://github.com/rails/rails
[4] Pull request 41598 in https://github.com/rails/rails
The `database:` argument from the `connected_to` method was deprecated
in Rails 6.1 [1] and removed in Rails 7.0 [2]. The ros-apartement gem
has already introduced fixes for this issue [3][4], but there has been
no release including these fixes.
So we're applying the fix in our code.
Note that, since Apartment already overwrites the `connected_to` method
by creating a `connected_to_with_tenant` method, we're calling the
`connected_to_without_tenant` method inside the patch, which is
equivalent to ActiveRecord's original `connected_to`.
[1] Pull request 37874 in https://github.com/rails/rails
[2] Pull request 40530 in https://github.com/rails/rails/pull
[3] Pull request 194 in https://github.com/rails-on-services/apartment
[4] Pull request 243 in https://github.com/rails-on-services/apartment
Byebug hasn't been maintained for years, and it isn't fully compatible
with Zeitwerk [1]. On the other hand, Ruby includes the debug gem since
version 3.1.0. We tried to start using at after commit e74eff217, but
couldn't do so because our CI was hanging forever in a test related to
machine learning, with the message:
> DEBUGGER: Attaching after process X fork to child process Y
(Note this message appeared with debug 1.6.3 but not with the version
we're currently using.)
So we're changing the debug gem fork mode in the test so it doesn't hang
anymore when running our CI. We tried to change the test so it wouldn't
call `Process.fork`, but this required changing the code, and since
there are no tests checking machine learning behavior with real scripts,
we aren't sure whether these script would keep working after changing
the code.
[1] Issue 564 in https://github.com/deivid-rodriguez/byebug
These warnings appear in the logs in the development environment, and,
with Rails 7, the application will crash. When running the tests, they
would appear in the standard error ouput if we set `config.cache_classes
= false` in the test environment but, since that isn't the case, they
don't.
To reproduce these warnings (or the lack of them), start a Rails console
in development and check the log/development.log file.
Quoting from pull request 43508 in the Rails repository [1]:
> When you are running test locally, most of the time you run only a
> subset, so it's better to load as little code as possible to have a
> faster time to first test result.
>
> But when you are on CI, it's usually much preferable to eager load the
> whole application because you will likely need all the code anyway,
> and even if the test suite is split across runners, it's preferable to
> load all the code to ensure any codefile that may have side effects is
> loaded.
>
> This also ensure that if some autoloaded constants are not properly
> tested on CI, at least they'll be loaded and obvious errors (e.g.
> SyntaxError) will be caught on CI rather than during deploy.
[1] https://github.com/rails/rails/commit/db0ee287eed
While using `require_dependency` to load original Consul Democracy code
from custom code works with the classic autoloader, this method was
never meant to be used this way. With zeitwerk, the code (apparently)
works in the test, development and production environments, but there's
one important gotcha: changing any `.rb` file in development will
require restarting the rails server since the application will crash
when reloading.
Quoting zeitwerk's author Xavier Noria, whom we've contacted while
looking for a solution:
> With the classic autoloader, when the Setting constant is autoloaded,
> the autoloader searched the autoload paths, found setting.rb in
> app/models/custom, and loaded it. With zeitwerk, the autoloader scans
> the folders in order and defines an autoload (Module#autoload) in
> Object so Ruby autoloads Setting with app/models/custom/settings.rb.
> Later, when app/models/setting.rb is found, it's ignored since there's
> already an autoload for Setting.
>
> That means the first file is managed by the autoloaders, while the
> second is not.
>
> So require_dependency worked, but it was pure luck, since the purpose
> of require_dependency is forcing the load of files managed by the
> autoloaders and, as we've seen, app/models/settings.rb isn't one of
> them.
>
> With your current pattern for custom files, the best solution is using
> Kernel#load.
So we're using `load` instead of `require_dependency`. Note that, with
`load`, we need to add the `.rb` extension to the required file, and we
no longer have to convert the Pathname to a string with `to_s`.
In Rails 6.1, the classic autoloader is deprecated.
We were getting an error because we were using `autoload` in the
ActiveStorage plugin for CKEditor:
expected file app/lib/ckeditor/backend/active_storage.rb to define
constant Ckeditor::Backend::ActiveStorage
So we're removing the line causing the error.
Finally, we can now restore all the tests that that failed sometimes
with the classic autoloader and that we modified in commits 2af1fc72f
and 8ba37b295.
In order for `include SentencesParser` to work with Zeitwerk, we'd have
to change the code slightly, so it follows Ruby conventions to resolve
constants:
```
module RemoteTranslations::Microsoft
class Client
include SentencesParser
# (...)
end
end
```
This would mean changing the indentation of the whole file. While we can
do that, changing the indentation of a file makes it harder to use
commands like `git blame` or `git log` with the file, so we're doing the
change the easy way.
Since we're already setting `wordpress_oauth2` using the `option :name`
command in the `OmniAuth::Strategies::Wordpress` class, Devise can
automatically find the strategy. However, it wasn't working because we
were passing a string instead of a symbol.
Even though we don't load this file with Zeitwerk, we're doing it for
consistency.
If we tried to load this file using Zeitwerk, without this change we'd
get an error:
```
NameError: uninitialized constant OmniauthWordpress
```
We were getting a few errors when trying out Zeitwerk:
```
expected file lib/sms_api.rb to define constant SmsApi
expected file app/components/layout/common_html_attributes_component.rb
to define constant Layout::CommonHtmlAttributesComponent
```
In these cases, we aren't using an inflection because we also define the
`Verification::SmsController` and a few migrations containing `Html` in
their class name, and none of them would work if we defined the
inflection.
We were also getting an error regarding classes containing WYSIWYG in
its name:
```
NameError: uninitialized constant WYSIWYGSanitizer
Did you mean? WysiwygSanitizer
```
In this case, adding the acronym is easier, since we never use "Wysiwyg"
in the code but we use "WYSIWYG" in many places.
The purpose of the lib folder is to have code that doesn't necessary
belong in the application but can be shared with other applications.
However, we don't have other applications and, if we did, the way to
share code between them would be using a gem or even a git submodule.
So having both the `app/` and the `lib/` folders is confusing IMHO, and
it causes unnecessary problems with autoloading.
So we're moving the `lib/` folder to `app/lib/`. Originally, some of
these files were in the `app/services/` folder and then they were moved
to the `lib/` folder. We're using `app/lib/` instead of `app/services/`
so the upgrade is less confusing.
There's an exception, though. The `OmniAuth::Strategies::Wordpress`
class needs to be available in the Devise initializer. Since this is an
initializer and trying to autoload a class here will be problematic when
switching to Zeitwerk, we'll keep the `require` clause on top of the
Devise initializer in order to load the file and so it will be loaded
even if it isn't in the autoload paths anymore.
This monkey-patch doesn't seem to be working with Zeitwerk, and we were
only using it in one place, so the easiest way to solve the problem is
to remove it.
Note that, in the process, we're changing the operation so `* 100`
appears before the division, so it's consistent with other places where
we do similar things (like the `supports_percentage` method in the
proposals helper).
We use vendor/assets and app/assets; the purpose of lib/assets isn't
that clear, though. According to the Rails guides:
> lib/assets is for your own libraries' code that doesn't really fit
> into the scope of the application or those libraries which are shared
> across applications.
So it must be something for companies having several Rails applications,
which isn't our case. Furthermore, this text has been removed from the
Rails guides for version 7.1, so this folder might be a legacy folder.
Reading conditions in Ruby is much easier than reading them in ERB and,
since the block only had only HTML tag (the <span> tag for deleted
users) but was using Ruby in all other four cases, we're moving it to a
Ruby file.
The `alt` attribute is mandatory in image tags. In this case, we're
leaving it empty because we also display text showing whether comments
are made by administrators, moderators or organizations.
The initialjs-rails gem hasn't been maintained for years, and it
currently requires `railties < 7.0`, meaning we can't upgrade to Rails 7
while we depend on it.
Since the code in the gem is simple, and we were already rewriting its
most complex part (generating a background color), we can implement the
same code, only we're using Ruby instead of JavaScript. This way, the
avatars will be shown on browsers without JavaScript as well. Since
we're adding a component test that checks SVG images are displayed even
without JavaScript, we no longer need the test that checked images were
displayed after AJAX requests.
Now the tests show the user experience better; people don't care about
the internal name used to select the initial (which is what we were
checking); they care about the initial actually displayed.
Note initialjs generated an <img> tag using a `src="data:image/svg+xml;`
attribute. We're generating an <svg> tag instead, because it's easier.
For this reason, we need to change the code slightly, giving the <svg>
tag the `img` role and using `aria-label` so its contents won't be read
aloud by screen readers. We could give it a `presentation` role instead
and forget about `aria-label`, but then screen readers would read the
text anyway (or, at least, some of them would).
These images are always displayed next to a username, meaning people
using screen readers were hearing the same username twice in a row.
Even though we're about to replace the initialjs gem, we're making this
change in case so we've got one more test and we can check everything
keeps working after replacing the gem.
We were using generic names like `args` and `options` which don't really
add anything to `*` or `**` because Ruby required us to.
That's no longer the case in Ruby 3.2, so we can simplify the code a
bit.
As usual, we're updating the bundler version in our Gemfile.lock so it
uses the one included in Ruby 3.2.3, and we're also updating the
`parser` gem so it supports this version.
Note that the `budget` parameter was added to the `delete_path` method
so it works in the tests; on production, it worked because this
component is only rendered on pages which already have the `budget`
parameter.
Co-authored-by: Javi Martín <javim@elretirao.net>
We're using `@extend` with a placeholder selector to generate the code
related to the icons. That means the generated CSS code will look
similar to:
```
.something,
.something-else,
.in-favor-against button:not(:hover, :active),
.etcetera,
.more-etcetera {
/* Rules here */
}
```
That means that, if one selector isn't supported by the browser, none of
the specified selectors will apply these rules.
The `:not(:hover, :active)` selector, introduced in commit 3482e6e05, is
currently supported by 96%-98% of the browsers. Browsers like Internet
Explorer don't support it.
Since there's a simple solution for this issue which results in a big
gain for 2%-4% of the population, we're fixing the issue by avoiding the
non-universally supported selector.
Foundation added compatibility with Dart Sass by implementing a `divide`
function and using it instead of `/` to perform divisions [1]. However,
this made CSS compilation much slower, with the cause being the usage of
the `divide` function inside Foundation's recursive `nth-root` and `pow`
functions. Since the `nth-root` function is only called by the `pow`
function, overriding the `pow` function so it uses the `math.pow`
function provided by Dart Sass solves the issue.
[1] Pull request 12241 in https://github.com/foundation/foundation-sites
Note that the sticky plugin no longer works with `data-top-anchor="0"`.
Quoting from the Foundation documentation:
> It's important to note that sticky requires a bit of developer input
> to work properly. (...) It's also important to set the minimum
> top-anchor point to 1px, otherwise it'll never stick!
Also note that the foundation-sites package already depends on the
motion-ui package, so we don't have to explicitly include this
dependency anymore. Since now we're using Dart Sass, we can upgrade to
motion-ui 2.0.5.
Since this new version already defines variables before using `!global`
with them, we can remove the changes we did in commit 1e1edc02e.
Finally, note we aren't removing the "upgrade Foundation" part of the
comment in `config/initializers/sass.rb` because we're still getting one
Dart Sass warning due to Foundation's code:
```
Deprecation Warning: Passing percentage units to the global abs()
function is deprecated.
In the future, this will emit a CSS abs() function to be by the
ser.
To preserve current behavior: math.abs(100%)
To emit a CSS abs() now: abs(#{100%})
$divisor: abs($divisor);
```
This warning will be removed when we upgrade to Foundation 6.8.1. We
aren't upgrading to that version now for the same reason we don't
upgrade two minor Rails versions at once: it would increase the chance
of breaking something.
